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

feat(real-hoist): crate skeleton + lockfile-to-HoisterTree wrapper (#438 slice 3a)#448

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

feat(real-hoist): crate skeleton + lockfile-to-HoisterTree wrapper (#438 slice 3a)#448
zkochan merged 3 commits into
mainfrom
feat/438-slice-3

Conversation

@zkochan

@zkochan zkochan commented May 13, 2026

Copy link
Copy Markdown
Member

Summary

First sub-slice of the Slice 3 hoister port from #438. Lands the IO
surface and the pnpm-side wrapper around the hoist algorithm; the
inner @yarnpkg/nm algorithm is stubbed (returns the input tree
unchanged) and replaced in 3b.

The umbrella's original Slice 3 — a faithful port of the full
@yarnpkg/nm/hoist.ts (1032 LOC) — is being split into five
sub-slices so each one is reviewable on its own. Sub-slice 3a covers
the wrapper's IO and the construction-time error surface; 3b drops in
the single-round hoist core; 3c adds peer-constraint handling; 3d adds
multi-round convergence + soft-link; 3e wires hoistingLimits /
externalDependencies knobs through and ports the full upstream test
fixture set.

New crate pacquet-real-hoist

Mirrors upstream's installing/linking/real-hoist workspace package. Exposes:

  • HoisterTree, HoisterResult, HoisterDependencyKind,
    HoistingLimits, HoistOpts, HoistError — the public surface
    the hoister contract speaks in.
  • RcByPtr<T> — identity-hashed Rc<T> wrapper used as children of
    IndexSets so a node added via two parent paths stays shared.
    Matches JS Set<HoisterTree> semantics (object-identity dedup)
    without the cost of structural hashing on deep trees.
  • hoist(&Lockfile, &HoistOpts) -> Result<HoisterResult, HoistError>
    — the pnpm wrapper itself. Builds the HoisterTree (root importer's
    merged dependencies + devDependencies + optionalDependencies,
    recursive descent through snapshots, plus link: placeholders
    for externalDependencies and workspace-importer children), runs
    the inner stub, then post-filters externalDependencies out of
    the top-level result.

HoistError::LockfileMissingDependency surfaces with miette code
ERR_PNPM_LOCKFILE_MISSING_DEPENDENCY (pnpm.io/errors) — same code upstream uses.

Upstream reference

Ports/cites pinned at pnpm/pnpm@94240bc0 and yarnpkg/berry@4287909fa6:

Test plan

Five unit tests covering the wrapper's observable behaviour:

  • hoist_throws_on_broken_lockfile — port of upstream's
    "hoist throws an error if the lockfile is broken" test.
  • empty_lockfile_yields_empty_root — wrapper's "no root
    importer" branch.
  • one_transitive_dep_appears_under_its_parent_in_the_stub
    pins the current shape so 3b's algorithm replacement is
    observably correct (the tree will go from root → a → b to
    root → {a, b} once real hoisting lands).
  • shared_dep_via_two_root_paths_is_one_node — pins the dedup
    cache; future refactors of the nodes HashMap won't silently
    lose the identity sharing the real algorithm relies on.
  • external_dependencies_are_stripped_from_the_result.
  • just ready, cargo doc --document-private-items,
    taplo format --check, just dylint all clean.

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

Summary by CodeRabbit

  • New Features

    • Added a new hoisting component to the workspace with a public hoist API, configurable hoist options (limits, external dependencies, peer handling), deterministic dependency resolution, and clear error reporting for missing lockfile entries.
  • Tests

    • Added unit tests covering alias resolution, missing-snapshot errors, empty lockfiles, basic hoist shapes, node deduplication, and stripping of external placeholders.

Review Change Stack

 slice 3a)

First sub-slice of the Slice 3 hoister port from the umbrella. Lands
the IO surface and the pnpm-side wrapper; the inner `@yarnpkg/nm`
algorithm is stubbed (returns the input tree unchanged) and replaced
in 3b.

New crate `pacquet-real-hoist`:

- `HoisterTree` / `HoisterResult` / `HoisterDependencyKind` /
  `HoistingLimits` / `HoistOpts` / `HoistError`, mirroring upstream's
  `@yarnpkg/nm` types and the pnpm wrapper's option object.
- `RcByPtr<T>` wrapper providing identity-hashed `Rc<T>` so children
  stored in `IndexSet` keep JS `Set<HoisterTree>` semantics (a node
  added via two parent paths stays shared) without paying the cost
  of structural hashing.
- `hoist(&Lockfile, &HoistOpts)` — ports the wrapper at
  `installing/linking/real-hoist/src/index.ts`. Builds the
  HoisterTree from the lockfile (root importer's
  dependencies/devDependencies/optionalDependencies merged, then
  recursive descent through `snapshots`), adds workspace importer
  children, plus `link:` placeholders for `externalDependencies`,
  runs the inner stub, and post-filters `externalDependencies` out
  of the top-level result.
- `LockfileMissingDependency` error surfaced as a `miette` diagnostic
  with code `ERR_PNPM_LOCKFILE_MISSING_DEPENDENCY`, matching the
  upstream error code at https://pnpm.io/errors.

Five unit tests pin the wrapper's observable behaviour:

- The "broken lockfile" error case is a direct port of pnpm's
  `installing/linking/real-hoist/test/index.ts` test.
- An empty lockfile yields an empty root.
- A `root → a → b` lockfile, under the stub, gives `root → a → b`
  (nothing hoisted). Pinning this means 3b's algorithm replacement
  is observably correct — the tree shape will change to
  `root → {a, b}` once real hoisting lands.
- A diamond dep (`root → {a, c} → b`) keeps `b` as one identity
  through both parents (proves the wrapper's dedup cache).
- `external_dependencies` are stripped from the result.

Upstream references pin `pnpm/pnpm@94240bc0` and
`yarnpkg/berry@4287909fa6`.
Copilot AI review requested due to automatic review settings May 13, 2026 12:04
@coderabbitai

coderabbitai Bot commented May 13, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 986d934c-b42f-475d-b8aa-866f0b95ced9

📥 Commits

Reviewing files that changed from the base of the PR and between c67430b and 8e92fdb.

📒 Files selected for processing (2)
  • crates/real-hoist/src/lib.rs
  • crates/real-hoist/src/tests.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • crates/real-hoist/src/tests.rs
  • crates/real-hoist/src/lib.rs
📜 Recent 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). (9)
  • GitHub Check: Cargo Deny
  • GitHub Check: Dylint
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Lint and Test (ubuntu-latest)
  • GitHub Check: Doc
  • GitHub Check: Lint and Test (windows-latest)
  • GitHub Check: Lint and Test (macos-latest)
  • GitHub Check: Code Coverage
  • GitHub Check: Run benchmark on ubuntu-latest

📝 Walkthrough

Walkthrough

Adds a new crate pacquet-real-hoist to the workspace that builds deterministic hoister trees from pnpm lockfiles, exposes public hoister types and options, implements hoist orchestration with cycle-safe memoization and pointer-identity deduplication, and includes unit tests and a stubbed nm-style conversion.

Changes

pacquet-real-hoist crate implementation

Layer / File(s) Summary
Workspace integration and crate manifest
Cargo.toml, crates/real-hoist/Cargo.toml
Registers pacquet-real-hoist in workspace dependencies and adds the crate manifest with dependencies on pacquet-lockfile, derive_more, indexmap, and miette (dev-dep pretty_assertions).
Public API contracts and data types
crates/real-hoist/src/lib.rs (lines 1–213)
Defines HoisterDependencyKind, HoisterTree, HoisterResult, HoistOpts/HoistingLimits, HoistError::LockfileMissingDependency, and RcByPtr<T> for pointer-identity deduplication.
Hoist orchestration and dependency resolution
crates/real-hoist/src/lib.rs (lines 214–420)
Implements hoist() orchestration, collect_importer_deps(), build_dep_node() (snapshot resolution, caching, peer-name computation, alias handling), and collect_snapshot_deps() with deterministic ordering and in-place dependency population.
Path encoding and graph conversion
crates/real-hoist/src/lib.rs (lines 421–544)
Adds percent_encode_path() and nm_hoist()/convert() stub that translate HoisterTree to HoisterResult while preserving shared/cyclic identity via pointer-keyed memoization.
Test suite and validation
crates/real-hoist/src/tests.rs
Test helpers and tests for missing-snapshot errors, empty lockfile, minimal chain (root -> a -> b), Rc pointer deduplication, external-dependency placeholder removal, and npm-alias transitive-resolution regression.

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant Hoist as hoist()
  participant Collector as collect_importer_deps()
  participant Builder as build_dep_node()
  participant Memo as memo (cache)
  participant NM as nm_hoist()
  Caller->>Hoist: call hoist(lockfile, opts)
  Hoist->>Collector: collect importer deps & inject externals
  Collector->>Builder: resolve alias -> build_dep_node()
  Builder->>Memo: check/insert placeholder for cycles
  Builder->>Builder: collect_snapshot_deps() recursively
  Hoist->>NM: nm_hoist(HoisterTree)
  NM->>Hoist: return HoisterResult
  Hoist->>Caller: return Result (strip external placeholders)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

Suggested reviewers

  • anthonyshew

Poem

🐰
I hopped through snapshots, names, and trees,
Stashing peers inside RC leaves,
Pointers match where cycles creep,
Hoist returns the tree I keep,
A tiny rabbit's code-time leap.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: adding a new crate skeleton and wrapper for the lockfile-to-HoisterTree conversion, which is the primary objective of this PR.
Description check ✅ Passed The description comprehensively covers the changes, includes upstream references with commit SHAs, documents the test plan with checkboxes, and addresses the split-slice approach. All template sections are addressed appropriately.
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.

✏️ 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-3

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

@codecov

codecov Bot commented May 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 78.09524% with 46 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.21%. Comparing base (71ad815) to head (8e92fdb).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
crates/real-hoist/src/lib.rs 78.09% 46 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #448      +/-   ##
==========================================
+ Coverage   87.04%   87.21%   +0.17%     
==========================================
  Files          96      101       +5     
  Lines        7246     8075     +829     
==========================================
+ Hits         6307     7043     +736     
- Misses        939     1032      +93     

☔ 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.

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

Introduces the new pacquet-real-hoist crate as the (currently stubbed) “real hoist” wrapper for nodeLinker: hoisted, translating a Lockfile into a HoisterTree, running a placeholder hoist pass, and returning a HoisterResult. This is positioned as sub-slice 3a of the broader hoisted-linker effort (#438), focusing on IO types + lockfile-to-tree construction and the wrapper’s error surface.

Changes:

  • Added new pacquet-real-hoist crate with public hoister IO types and hoist(&Lockfile, &HoistOpts) wrapper.
  • Implemented lockfile importer/snapshot traversal to build the hoister input tree, plus external_dependencies placeholder reservation + stripping.
  • Added unit tests pinning wrapper behavior and identity-dedup semantics.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
crates/real-hoist/src/lib.rs New crate implementation: public hoister types, lockfile-to-tree builder, stub hoist conversion, and error type.
crates/real-hoist/src/tests.rs Unit tests covering wrapper behavior, missing-snapshot error surfacing, identity dedup, and external dependency stripping.
crates/real-hoist/Cargo.toml New crate manifest and dependencies.
Cargo.toml Adds pacquet-real-hoist to workspace dependencies.
Cargo.lock Records the new crate in the lockfile.

💡 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
Comment thread crates/real-hoist/src/lib.rs Outdated
Comment thread crates/real-hoist/src/lib.rs
@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.4±0.57ms   264.6 KB/sec    1.00     16.3±0.28ms   265.7 KB/sec

Slice and sub-slice numbering is PR-organisation scaffolding — it's
useful in commit messages and PR descriptions but it rots inside
committed code the moment the referenced slices land, get renumbered,
or are abandoned. Rewrite the relevant doc-comments to describe what
the code *is* today rather than where it sits in a multi-PR sequence:

- Top-level module doc drops the "This is Sub-slice 3a of #438" intro.
- `hoist` doc drops the "Sub-slice 3a pins / later sub-slices replace"
  framing.
- `nm_hoist` stub doc drops "Sub-slice 3b replaces this".
- `external_dependencies` / non-root importer comments drop their
  "umbrella's Slice 10 / Slice 9" pointers.
- One test doc rewords "Sub-slice 3b's algorithm replacement is
  observably correct" into a description of what the test pins
  without naming the future slice.

@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 (2)
crates/real-hoist/src/lib.rs (2)

334-334: ⚡ Quick win

Use Rc::clone(&existing) for visibility.

The guidelines specify preferring Rc::clone(&x) over x.clone() for reference-counted types to make the cost visible at the call site. As per coding guidelines: "Prefer Arc::clone(&x) and Rc::clone(&x) over x.clone() for reference-counted types to make the cost visible at the call site."

♻️ Suggested change
-        return Ok(Rc::clone(existing));
+        return Ok(Rc::clone(&existing));
🤖 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` at line 334, The return currently calls
Rc::clone(existing) which hides the clone cost; change it to call
Rc::clone(&existing) so the reference-counted clone is explicit at the call
site—update the return expression that uses Rc::clone and the local variable
existing accordingly (i.e., pass &existing to Rc::clone).

497-497: ⚡ Quick win

Use Rc::clone(&existing) for visibility.

Same issue as line 334: prefer Rc::clone(&x) over x.clone() for reference-counted types per coding guidelines.

♻️ Suggested change
-        return Rc::clone(existing);
+        return Rc::clone(&existing);
🤖 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` at line 497, The return statement currently
calls Rc::clone(existing); change it to use a reference per project style by
calling Rc::clone(&existing) instead; locate the return that returns the cloned
Rc for the variable named existing (the same pattern as at the earlier
occurrence around line 334) and update that call to pass a reference to
existing.
🤖 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`:
- Line 334: The return currently calls Rc::clone(existing) which hides the clone
cost; change it to call Rc::clone(&existing) so the reference-counted clone is
explicit at the call site—update the return expression that uses Rc::clone and
the local variable existing accordingly (i.e., pass &existing to Rc::clone).
- Line 497: The return statement currently calls Rc::clone(existing); change it
to use a reference per project style by calling Rc::clone(&existing) instead;
locate the return that returns the cloned Rc for the variable named existing
(the same pattern as at the earlier occurrence around line 334) and update that
call to pass a reference to existing.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4e2a397f-1dca-4b14-b0fe-3e56c6a3933c

📥 Commits

Reviewing files that changed from the base of the PR and between 6ca5332 and e58c821.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (4)
  • Cargo.toml
  • crates/real-hoist/Cargo.toml
  • crates/real-hoist/src/lib.rs
  • crates/real-hoist/src/tests.rs
📜 Review details
🧰 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 (25)
crates/real-hoist/Cargo.toml (1)

1-22: LGTM!

Cargo.toml (1)

34-34: LGTM!

crates/real-hoist/src/lib.rs (16)

20-27: LGTM!


29-49: LGTM!


51-89: LGTM!


91-105: LGTM!


107-114: LGTM!


116-128: LGTM!


130-149: LGTM!


151-198: LGTM!


211-288: LGTM!


290-322: LGTM!


337-409: LGTM!


411-432: LGTM!


434-439: LGTM!


441-480: LGTM!


499-534: LGTM!


536-537: LGTM!

crates/real-hoist/src/tests.rs (7)

1-7: LGTM!


9-38: LGTM!


47-69: LGTM!


74-81: LGTM!


90-127: LGTM!


134-179: LGTM!


186-215: LGTM!

@github-actions

Copy link
Copy Markdown

Integrated-Benchmark Report (Linux)

Scenario: Frozen Lockfile

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 2.468 ± 0.088 2.327 2.623 1.02 ± 0.05
pacquet@main 2.425 ± 0.075 2.330 2.593 1.00
pnpm 5.746 ± 0.110 5.575 5.957 2.37 ± 0.09
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 2.46846971504,
      "stddev": 0.08756429931353996,
      "median": 2.45118976524,
      "user": 2.6013509599999995,
      "system": 3.398308159999999,
      "min": 2.32747669974,
      "max": 2.6230195487400003,
      "times": [
        2.45165180474,
        2.37711763074,
        2.6230195487400003,
        2.53186183274,
        2.44216262574,
        2.5673126437400002,
        2.48444720974,
        2.42891942874,
        2.32747669974,
        2.45072772574
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 2.42491967764,
      "stddev": 0.07500880341391536,
      "median": 2.43614090374,
      "user": 2.5920244599999998,
      "system": 3.4000415599999996,
      "min": 2.33027050774,
      "max": 2.59257235274,
      "times": [
        2.33027050774,
        2.45034985474,
        2.59257235274,
        2.3777098257400002,
        2.44755325474,
        2.4484370487400002,
        2.33224666074,
        2.44130145874,
        2.39777546374,
        2.43098034874
      ]
    },
    {
      "command": "pnpm",
      "mean": 5.74589620494,
      "stddev": 0.1095422989671101,
      "median": 5.74484399424,
      "user": 8.260028259999999,
      "system": 4.267844859999999,
      "min": 5.57465318974,
      "max": 5.95667776174,
      "times": [
        5.77470164874,
        5.66325593774,
        5.57465318974,
        5.77170584474,
        5.95667776174,
        5.87349349874,
        5.74123683874,
        5.66684232774,
        5.74845114974,
        5.68794385174
      ]
    }
  ]
}

Scenario: Frozen Lockfile (Hot Cache)

Command Mean [ms] Min [ms] Max [ms] Relative
pacquet@HEAD 682.1 ± 49.1 637.9 809.8 1.00 ± 0.08
pacquet@main 681.1 ± 26.4 652.4 724.5 1.00
pnpm 2430.7 ± 128.2 2324.3 2682.3 3.57 ± 0.23
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 0.68212582268,
      "stddev": 0.04910193651149939,
      "median": 0.6648381758799999,
      "user": 0.3401931,
      "system": 1.49230928,
      "min": 0.6379466338800001,
      "max": 0.8098443508800001,
      "times": [
        0.8098443508800001,
        0.66750053388,
        0.65635761588,
        0.70117992688,
        0.6531812058800001,
        0.66217581788,
        0.67822766688,
        0.69862345588,
        0.65622101888,
        0.6379466338800001
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 0.68106939128,
      "stddev": 0.026362252500029496,
      "median": 0.66949407988,
      "user": 0.34268,
      "system": 1.50263188,
      "min": 0.6524222388800001,
      "max": 0.72450306588,
      "times": [
        0.72035244688,
        0.66730836788,
        0.65670315088,
        0.70239793888,
        0.66320788488,
        0.68848994388,
        0.67167979188,
        0.72450306588,
        0.66362908288,
        0.6524222388800001
      ]
    },
    {
      "command": "pnpm",
      "mean": 2.4306578795799996,
      "stddev": 0.1282440979739961,
      "median": 2.37921015738,
      "user": 2.7939405999999996,
      "system": 2.1550779799999997,
      "min": 2.32425675288,
      "max": 2.68230174988,
      "times": [
        2.55697234688,
        2.32578541188,
        2.37429146788,
        2.68230174988,
        2.3435663738800003,
        2.32425675288,
        2.39830762788,
        2.33419144788,
        2.58277676988,
        2.38412884688
      ]
    }
  ]
}

…_exhaustive HoistError (#448 review)

Address four Copilot review findings on the new crate:

1. `collect_snapshot_deps` looked up `SnapshotDepRef::Alias` deps
   under the alias name instead of the resolved target name. The
   snapshot key for an npm-alias is `<target>@<ver>`, not
   `<alias>@<ver>`, so any real aliased transitive would have
   surfaced as `LockfileMissingDependency`. `build_dep_node` now
   takes the resolved `&PkgNameVerPeer` directly and the caller
   builds it via `dep_ref.resolve(alias)` for snapshot deps or
   `PkgNameVerPeer::new(alias, spec.version)` for importer deps.

2. `build_dep_node`'s cycle handling re-allocated the node on the
   way out (placeholder Rc, recurse, *new* finished Rc) which left
   any cycle-visiting Rc pointing at the empty placeholder.
   `HoisterTree::dependencies` is now a `RefCell<IndexSet<...>>` and
   the construction populates the cell in place — the placeholder
   and the populated node are the same allocation, so a back-edge
   reads the eventually-populated set. Same JS `Set<HoisterTree>`
   mutation semantics the real hoist algorithm needs.

3. `convert` had the same bug and gets the same fix on
   `HoisterResult::dependencies` and `HoisterResult::references`.
   The stub's traversal now collects the input children into a Vec
   before recursing so the borrow on the input cell drops, and
   populates the output cell in place.

4. `HoistError` is now `#[non_exhaustive]`, matching the rest of
   pacquet's public error enums.

A new regression test (`transitive_npm_alias_resolves_target_snapshot`)
pins the fix for (1): the snapshot key the wrapper looks up matches
the target package, the node's exposed `name` stays the alias, and
`ident_name` / `reference` carry the resolved target's identity.

Existing tests updated to take `Ref<'_, …>` via `.borrow()`. All six
pass; `just ready`, `cargo doc --document-private-items`, `taplo`,
and `just dylint` clean.
Copilot AI review requested due to automatic review settings May 13, 2026 12:28
@zkochan zkochan merged commit 43e5db9 into main May 13, 2026
15 checks passed
@zkochan zkochan deleted the feat/438-slice-3 branch May 13, 2026 12:33

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

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.

Comment on lines +460 to +485
let mut out = String::with_capacity(s.len());
for ch in s.chars() {
match ch {
'A'..='Z'
| 'a'..='z'
| '0'..='9'
| '-'
| '_'
| '.'
| '!'
| '~'
| '*'
| '\''
| '('
| ')' => out.push(ch),
'/' => out.push_str("%2F"),
other => {
// Best-effort %xx encode for the ASCII subset we
// expect in importer ids. Anything else is left
// verbatim — pacquet's lockfile doesn't currently
// hand the wrapper non-ASCII paths.
if (other as u32) < 0x80 {
out.push_str(&format!("%{:02X}", other as u32));
} else {
out.push(other);
}
zkochan added a commit that referenced this pull request May 13, 2026
After rebasing on top of #448 (new `pacquet-real-hoist` crate), the
lockfile-to-HoisterTree wrapper consumed `spec.version` expecting a
`PkgVerPeer` — but this PR widens `ResolvedDependencySpec.version` to
`ImporterDepVersion` so `link:` (cross-importer `workspace:*`) deps
can be represented. `link:` deps don't live in the virtual store, so
they have no snapshot to hoist; mirror the same skip that
`pacquet-package-manager`'s `build_sequence::collect_root_dep_paths`
already does and `continue` past `ImporterDepVersion::Link` values.

The matching `resolved_dep` test helper takes a `&str` literal and is
unit-test infrastructure, so a `.into()` from `PkgVerPeer` to
`ImporterDepVersion::Regular` is enough — no functional change.

---
Written by an agent (Claude Code, claude-opus-4-7).
zkochan added a commit that referenced this pull request May 13, 2026
After rebasing on top of #448 (new `pacquet-real-hoist` crate), the
lockfile-to-HoisterTree wrapper consumed `spec.version` expecting a
`PkgVerPeer` — but this PR widens `ResolvedDependencySpec.version` to
`ImporterDepVersion` so `link:` (cross-importer `workspace:*`) deps
can be represented. `link:` deps don't live in the virtual store, so
they have no snapshot to hoist; mirror the same skip that
`pacquet-package-manager`'s `build_sequence::collect_root_dep_paths`
already does and `continue` past `ImporterDepVersion::Link` values.

The matching `resolved_dep` test helper takes a `&str` literal and is
unit-test infrastructure, so a `.into()` from `PkgVerPeer` to
`ImporterDepVersion::Regular` is enough — no functional change.

---
Written by an agent (Claude Code, claude-opus-4-7).
zkochan added a commit that referenced this pull request May 13, 2026
After rebasing on top of #448 (new `pacquet-real-hoist` crate), the
lockfile-to-HoisterTree wrapper consumed `spec.version` expecting a
`PkgVerPeer` — but this PR widens `ResolvedDependencySpec.version` to
`ImporterDepVersion` so `link:` (cross-importer `workspace:*`) deps
can be represented. `link:` deps don't live in the virtual store, so
they have no snapshot to hoist; mirror the same skip that
`pacquet-package-manager`'s `build_sequence::collect_root_dep_paths`
already does and `continue` past `ImporterDepVersion::Link` values.

The matching `resolved_dep` test helper takes a `&str` literal and is
unit-test infrastructure, so a `.into()` from `PkgVerPeer` to
`ImporterDepVersion::Regular` is enough — no functional change.

---
Written by an agent (Claude Code, claude-opus-4-7).
zkochan added a commit that referenced this pull request May 13, 2026
After rebasing on top of #448 (new `pacquet-real-hoist` crate), the
lockfile-to-HoisterTree wrapper consumed `spec.version` expecting a
`PkgVerPeer` — but this PR widens `ResolvedDependencySpec.version` to
`ImporterDepVersion` so `link:` (cross-importer `workspace:*`) deps
can be represented. `link:` deps don't live in the virtual store, so
they have no snapshot to hoist; mirror the same skip that
`pacquet-package-manager`'s `build_sequence::collect_root_dep_paths`
already does and `continue` past `ImporterDepVersion::Link` values.

The matching `resolved_dep` test helper takes a `&str` literal and is
unit-test infrastructure, so a `.into()` from `PkgVerPeer` to
`ImporterDepVersion::Regular` is enough — no functional change.

---
Written by an agent (Claude Code, claude-opus-4-7).
zkochan added a commit that referenced this pull request May 13, 2026
## Summary

Replaces the stub `nm_hoist` (landed in #448) with a working hoister and the surrounding guardrails: BFS over the result graph pulling every eligible descendant up to the root, plus upfront refusal of inputs the algorithm doesn't yet model.

### What the algorithm models

- **Free hoist.** A transitive dep with no name collision at the root surfaces at the root.
- **Identity dedup.** A dep reachable through multiple parents shares one `Rc` thanks to the wrapper's cache; the hoist preserves that identity and strips the duplicate reference at the other parent path.
- **Parent-wins on version conflict.** When two distinct deps share an alias but resolve to different snapshot keys, the first BFS visitor takes the root slot and the other stays under its declaring parent. Visit order matches the wrapper's alias-sorted insertion order, so the outcome is deterministic.
- **Deep chains flatten in one pass.** `root → a → b → c → d` becomes `root → {a, b, c, d}` — each node, once hoisted, is queued for further descent; its own children evaluate against root's slots rather than against the (now-empty) intermediate parent.
- **O(1) root-slot lookup.** A side `HashMap<String, RcByPtr<HoisterResult>>` mirrors root's direct deps, so the per-edge "is this name taken?" check doesn't degrade to O(N²) `IndexSet` scans on flat graphs.

### Fail-fast guards

The algorithm doesn't yet model peers, hoisting limits, or multi-importer (workspace) hoist trees. Rather than emit a layout pnpm would reject, the wrapper refuses these inputs upfront with three new `HoistError` variants:

- `UnsupportedPeerDependency { ident, peers }` — fires when scanning the constructed `HoisterTree` finds any node with non-empty `peer_names` (either `peerDependencies` from the `packages:` map or `transitive_peer_dependencies` on a `snapshots:` entry).
- `UnsupportedHoistingLimits { len }` — non-empty `opts.hoisting_limits`.
- `UnsupportedWorkspace { extra_importers }` — any importer beyond the root `.`.

Each carries enough context for an operator to identify what triggered it. The `UnsupportedWorkspace` help points at `SymlinkDirectDependencies` — workspaces *do* work in pacquet's wider install path (workspace support landed in #443 for the isolated linker); only the hoister is restricted.

### Rebase pickup

`collect_importer_deps` carries the `Link`-variant skip introduced by #443 (workspace support) — `spec.version.as_regular()` extracts the snapshot key for `Regular` deps and `continue`s past `Link` deps, since workspace siblings are direct symlinks materialised by `SymlinkDirectDependencies` and have no snapshot to hoist.

### Performance

Nothing in this PR is reachable from `pacquet install` today (`crates/package-manager/src/install*.rs` doesn't import `pacquet-real-hoist` — the crate is dead code from the install pipeline's perspective until the hoisted-linker wiring lands in later slices). The benchmark results bear that out: cold Frozen Lockfile is within CI variance (+3.2% mean, +3.5% median, driven by one outlier), Hot Cache is 6% *faster* (same `stat → lstat` improvement that shipped in slice 1's `import_indexed_dir`), micro is identical. No code path explains a real regression.

## What's deferred

- Peer-dependency-aware hoisting (`peer_names` constraints, peer-promise satisfaction).
- Multi-round convergence — the BFS handles deep chains in one pass, so the cases requiring true multi-round are limited to peer interactions.
- `hoistingLimits` runtime enforcement.
- `dependencyKind` distinctions for workspaces and external soft links (today only `ExternalSoftLink` placeholders are added by the wrapper and stripped post-hoist; `Workspace`-kind nodes are blocked by the `UnsupportedWorkspace` guard upfront).

## Upstream reference

- [`hoist.ts` algorithm overview](https://github.com/yarnpkg/berry/blob/4287909fa6a0a1ec976a55776bff606864b31990/packages/yarnpkg-nm/sources/hoist.ts#L1-L51) — the recipe pacquet's single-pass BFS approximates.
- [`hoistTo` main loop](https://github.com/yarnpkg/berry/blob/4287909fa6a0a1ec976a55776bff606864b31990/packages/yarnpkg-nm/sources/hoist.ts#L329) — the structural intent the port mirrors for the subset above.
@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