Skip to content
This repository was archived by the owner on May 14, 2026. It is now read-only.

feat(real-hoist): ancestor-path-aware peer-shadow refusal (#438)#461

Merged
zkochan merged 2 commits into
mainfrom
feat/438-slice-3c
May 13, 2026
Merged

feat(real-hoist): ancestor-path-aware peer-shadow refusal (#438)#461
zkochan merged 2 commits into
mainfrom
feat/438-slice-3c

Conversation

@zkochan

@zkochan zkochan commented May 13, 2026

Copy link
Copy Markdown
Member

Summary

Lifts the previous UnsupportedPeerDependency upfront guard and replaces 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.

How it works

  • HoisterResult carries the input peer_names set forward. Upstream's HoisterResult doesn't (peer info lives on its intermediate HoisterWorkTree); pacquet runs the algorithm on HoisterResult directly, so the peer set rides along.

  • The hoist driver is a recursive hoist_subtree that 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 with parent_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 by peer_check_uses_post_hoist_ancestor_path_not_queue_time_path.)

  • would_shadow_peer walks that path to decide whether a candidate hoist would change its own peer resolution.

  • New AbsorbDecision::PeerShadow variant alongside Free / SameNode / Conflict. Fires 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 upfront scan are gone. The #[non_exhaustive] 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 (its identity-dedup is a feature) and records only the path DFS actually used to reach the candidate. 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 ends up at most over-nested, never under-nested. The trade-off is documented on would_shadow_peer.

Upstream reference

Test plan

  • peer_constrained_node_stays_under_parent_when_root_provides_different_identapp → 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 but app → react@18 matches root's react@18 (shared Rc via 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) where mid hoists 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 post-hoist path [root, mid] finds no provider mismatch and terminal correctly flattens to root.
  • All 10 pre-existing tests still pass (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).
  • The previously-failing peer_dependency_in_lockfile_surfaces_unsupported test from feat(real-hoist): single-pass parent-wins hoist (#438) #452 is replaced (peers are no longer an unsupported input).
  • All Copilot review threads addressed and resolved (stale ancestor path, misleading comment).
  • just ready (836 tests pass), cargo doc --document-private-items, taplo format --check, just dylint all clean.

Written by an agent (Claude Code, claude-opus-4-7).

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).
Copilot AI review requested due to automatic review settings May 13, 2026 15:13
@coderabbitai

coderabbitai Bot commented May 13, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

The hoisting algorithm moves peer handling from an upfront scan into runtime DFS evaluation. HoisterResult gains a public peer_names field. hoist_into_root tracks ancestor paths and uses would_shadow_peer to upgrade Free to PeerShadow, refusing hoists that would change peer resolution. Tests cover conflict and agreement cases.

Changes

Peer-shadow hoist refusal in nm_hoist runtime

Layer / File(s) Summary
HoisterResult peer_names field
crates/real-hoist/src/lib.rs
HoisterResult gains public peer_names: BTreeSet<String> field to carry peer constraint names from the hoisting tree.
Remove upfront peer scanning
crates/real-hoist/src/lib.rs
Deletes the wrapper guard and find_first_peer_constrained helper that previously rejected peer-constrained nodes before hoisting.
Remove UnsupportedPeerDependency variant
crates/real-hoist/src/lib.rs
Removes HoistError::UnsupportedPeerDependency and its early-reject semantics.
Switch traversal to recursive DFS
crates/real-hoist/src/lib.rs
hoist_into_root is reworked from an iterative queue to a recursive DFS driver; unused VecDeque import removed.
Hoist subtree recursion and PeerShadow handling
crates/real-hoist/src/lib.rs
hoist_subtree threads ancestor_path, computes absorb decisions including PeerShadow, upgrades Free to PeerShadow where would_shadow_peer applies, and treats PeerShadow like Conflict while preserving SameNode deduplication.
Peer check logic (would_shadow_peer)
crates/real-hoist/src/lib.rs
Implements root-shadow guard and ancestor-path mismatch walk comparing ident resolution expectations to the provider installed at the root slot.
Wire peer_names through convert()
crates/real-hoist/src/lib.rs
convert() clones HoisterTree.peer_names into HoisterResult.peer_names.
Update nm_hoist documentation
crates/real-hoist/src/lib.rs
Docs updated to describe peer-shadow refusal behavior and remaining limitations.
Test helpers and peer-constrained hoisting coverage
crates/real-hoist/src/tests.rs
Adds pkg_metadata_with_peer helper and tests verifying peer-refusal, post-hoist ancestor-path correctness, peer-agreement hoisting, and updates an earlier unsupported-peer test to expect success.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

  • pnpm/pacquet#448: Related changes to crates/real-hoist/src/lib.rs and initial HoisterTree/HoisterResult wiring.
  • pnpm/pacquet#452: Prior peer-aware handling in nm_hoist and UnsupportedPeerDependency semantics that this PR replaces.

Poem

🐰 Hops through the hoisting spree—
Old peer-scan guard dismissed with glee,
Ancestor paths traced in the DFS tree,
Shadow checks keep peers as they should be,
A rabbit cheers: hoists guard harmony!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat(real-hoist): ancestor-path-aware peer-shadow refusal' directly describes the main change — implementing ancestor-path-aware peer-shadow refusal in the hoisting algorithm.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The PR description comprehensively covers the summary, implementation approach, upstream references, and test plan with clear examples.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/438-slice-3c

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
crates/real-hoist/src/lib.rs (1)

813-818: 💤 Low value

Prefer Rc::clone(&d.0) to make the refcount bump visible.

As per coding guidelines: "Prefer Rc::clone(&x) over x.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

📥 Commits

Reviewing files that changed from the base of the PR and between 99d708a and c69bd52.

📒 Files selected for processing (2)
  • crates/real-hoist/src/lib.rs
  • crates/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 and pipe-trait chains; do not break them into intermediate let bindings 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 (&Path over &PathBuf, &str over &String) when it does not force extra copies.
Prefer Arc::clone(&x) and Rc::clone(&x) over x.clone() for reference-counted types to make the cost visible at the call site.
Do not use star imports inside module bodies. Write use super::{Foo, bar} instead of use 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::*; in lib.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 plain String or &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 via TryFrom<String> and/or FromStr; 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 infallible From<String> and From<&str> constructor in the Rust wrapper.
When upstream TypeScript occasionally constructs a branded type without validation (via bare as assertion), add a from_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.rs
  • crates/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.rs
  • crates/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!

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 HoisterResult with peer_names and 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.

Comment thread crates/real-hoist/src/lib.rs Outdated
Comment thread crates/real-hoist/src/lib.rs Outdated
@github-actions

github-actions Bot commented May 13, 2026

Copy link
Copy Markdown

Micro-Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.00     16.0±0.33ms   270.7 KB/sec    1.00     16.0±0.30ms   271.6 KB/sec

@codecov

codecov Bot commented May 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.70130% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 87.34%. Comparing base (477d240) to head (12a32ea).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
crates/real-hoist/src/lib.rs 98.70% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

Copy link
Copy Markdown

Integrated-Benchmark Report (Linux)

Scenario: Frozen Lockfile

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 2.680 ± 0.118 2.562 2.904 1.03 ± 0.05
pacquet@main 2.594 ± 0.055 2.509 2.692 1.00
pnpm 6.527 ± 0.098 6.351 6.650 2.52 ± 0.07
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)

Command Mean [ms] Min [ms] Max [ms] Relative
pacquet@HEAD 706.4 ± 35.8 686.0 805.8 1.00
pacquet@main 744.6 ± 50.5 678.0 832.4 1.05 ± 0.09
pnpm 2748.4 ± 102.2 2620.5 2909.4 3.89 ± 0.24
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.
@zkochan zkochan merged commit 79ebc41 into main May 13, 2026
15 of 16 checks passed
@zkochan zkochan deleted the feat/438-slice-3c branch May 13, 2026 15:50
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.
@zkochan zkochan mentioned this pull request May 14, 2026
59 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants