Skip to content

fix(pacquet/lockfile): accept link: refs in snapshot dependencies#11788

Merged
zkochan merged 2 commits into
mainfrom
fix/11775
May 20, 2026
Merged

fix(pacquet/lockfile): accept link: refs in snapshot dependencies#11788
zkochan merged 2 commits into
mainfrom
fix/11775

Conversation

@zkochan

@zkochan zkochan commented May 20, 2026

Copy link
Copy Markdown
Member

Summary

  • SnapshotDepRef::FromStr only handled the bare-version and npm-alias shapes. A link:<path> value in a snapshot's dependencies: map (which pnpm writes when an injected workspace package's own deps resolve to a sibling workspace project) tripped the version parser and aborted the install.
  • Added a SnapshotDepRef::Link(String) variant mirroring ImporterDepVersion::Link. resolve() and ver_peer() now return Option, with None for the link case — matching upstream's refToRelative returning null for link: references.
  • Every call site (real-hoist, hoist, build_sequence, deps_graph, virtual_store_layout, create_symlink_layout, link_bins, hoisted_dep_graph, install_frozen_lockfile, current_lockfile) now skips link entries, mirroring upstream's if (childDepPath) guards in getChildren / lockfileDepsToGraphChildren.

Fixes #11775.

Test plan

  • Added a regression test in pacquet/crates/lockfile/src/load_lockfile/tests.rs that loads the lockfile shape from the issue report (b@file:packages/b snapshot with c: link:packages/c dependency) and asserts the dep parses as SnapshotDepRef::Link("packages/c") with resolve() returning None.
  • Extended snapshot_dep_ref/tests.rs with parse_link_workspace_path, resolve_link_returns_none, link cases in display_roundtrip / deserialize_ok, and the Link arm of ver_peer_returns_inner_version_for_each_variant.
  • cargo nextest run — 1720 passed, 3 skipped.
  • cargo clippy --locked --workspace --all-targets -- --deny warnings — clean.
  • cargo fmt --check — clean.

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

Summary by CodeRabbit

  • New Features

    • Lockfiles now recognize "link:" workspace dependencies as external targets and exclude them from snapshot graphs, hoisting, symlink generation, and automatic bin collection.
  • Bug Fixes

    • Build, hoist, install, and symlink flows now safely skip unresolved "link:" references to avoid incorrect graph entries and runtime errors.
  • Tests

    • Added tests for parsing, serialization round-trips, and handling of "link:" dependencies.

Review Change Stack

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@coderabbitai

coderabbitai Bot commented May 20, 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: d099e2c5-6137-40fa-a8ce-300a5a2aee4f

📥 Commits

Reviewing files that changed from the base of the PR and between d62284a and e1888c8.

📒 Files selected for processing (1)
  • pacquet/crates/lockfile/src/snapshot_dep_ref/tests.rs
💤 Files with no reviewable changes (1)
  • pacquet/crates/lockfile/src/snapshot_dep_ref/tests.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). (5)
  • GitHub Check: Lint and Test (windows-latest)
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Analyze (javascript)
  • GitHub Check: Code Coverage
  • GitHub Check: Compile & Lint

📝 Walkthrough

Walkthrough

Adds support for snapshot link: workspace paths (new SnapshotDepRef::Link), makes resolution/version access fallible for link refs, and updates downstream graph, hoist, layout, symlink, and bin logic to skip unresolved link dependencies.

Changes

Link Workspace Path Support

Layer / File(s) Summary
SnapshotDepRef type extension and core API
pacquet/crates/lockfile/src/snapshot_dep_ref.rs
SnapshotDepRef enum adds Link(String) variant; resolve() and ver_peer() return Option types; new as_link_target() accessor; Display and FromStr implementations updated for link: format.
SnapshotDepRef unit tests for link variant
pacquet/crates/lockfile/src/snapshot_dep_ref/tests.rs
Tests verify link: parsing, None resolution, None from ver_peer(), and as_link_target() path extraction. Covers string roundtrips and YAML deserialization.
Lockfile loading and link dependency regression test
pacquet/crates/lockfile/src/load_lockfile/tests.rs
Imports extended to include PkgName and SnapshotDepRef. New regression test constructs multi-package lockfile with injected workspace snapshot depending on another package via link:packages/c, verifying parse and resolution behavior.
Documentation clarification for link at snapshot level
pacquet/crates/lockfile/src/resolved_dependency.rs
Docstring updated to clarify that link: dependencies can appear at snapshot level (injected workspace packages), not only at importer level.
Graph and layout builders handle optional resolution
pacquet/crates/package-manager/src/build_sequence.rs, pacquet/crates/package-manager/src/deps_graph.rs, pacquet/crates/package-manager/src/hoist.rs, pacquet/crates/package-manager/src/hoisted_dep_graph.rs, pacquet/crates/package-manager/src/virtual_store_layout.rs, pacquet/crates/real-hoist/src/lib.rs, pacquet/crates/package-manager/src/current_lockfile.rs
Dependency resolution now guards on Some() before adding to build adjacency graph, dependency graph, hoist graph, reachability traversal, and hoisted-graph children. Unresolved deps (notably link:) are excluded from snapshot-driven traversals.
Symlink and bin linking handle optional resolution
pacquet/crates/package-manager/src/create_symlink_layout.rs, pacquet/crates/package-manager/src/link_bins.rs, pacquet/crates/package-manager/src/install_frozen_lockfile.rs
Dependency resolution guards on optional results. Unresolved deps skip symlink generation, are excluded from bin candidate sets, and do not contribute to runtime node detection.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • pnpm/pnpm#11710: Adds/extends unit tests around lockfile::snapshot_dep_ref and related resolution behaviors that overlap with this change.

Poem

"I sniffed a link path down the trail,
A workspace hop, a small brown tail.
When resolve said 'None' I didn't fret,
I skipped and hopped — no graph upset. 🐇✨"

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: adding support for link: refs in snapshot dependencies, which directly addresses the parsing failure described in the PR objectives.
Linked Issues check ✅ Passed The PR fully addresses issue #11775 requirements: accepts link: entries in lockfile snapshots, enables frozen installs with link: workspace dependencies, implements non-resolvable semantics matching upstream, and includes regression tests.
Out of Scope Changes check ✅ Passed All changes are scoped to supporting link: refs in snapshot dependencies. Updates to parsing, resolution logic, call sites, and tests are all directly necessary to fulfill the linked issue requirements.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ 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 fix/11775

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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)
pacquet/crates/lockfile/src/snapshot_dep_ref/tests.rs (1)

61-64: ⚡ Quick win

Trim test doc-comments that restate assertions.

These blocks largely repeat what the test names and assertions already express. Consider reducing them to brief non-obvious context (or removing them) to keep test intent crisp.

As per coding guidelines: "Tests are documentation. Do not duplicate them in prose."

Also applies to: 100-105

🤖 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 `@pacquet/crates/lockfile/src/snapshot_dep_ref/tests.rs` around lines 61 - 64,
The doc-comment blocks that restate test assertions (e.g., the block starting
"`link:` deps live outside the virtual store..." and the similar block around
the 100-105 region) should be trimmed or removed so the test name and assertions
are the primary documentation; replace verbose prose with a short one-line note
only if it adds non-obvious context, otherwise delete the comment lines above
the test functions in snapshot_dep_ref/tests.rs so the tests remain concise and
still compile/run.
🤖 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 `@pacquet/crates/lockfile/src/snapshot_dep_ref/tests.rs`:
- Around line 61-64: The doc-comment blocks that restate test assertions (e.g.,
the block starting "`link:` deps live outside the virtual store..." and the
similar block around the 100-105 region) should be trimmed or removed so the
test name and assertions are the primary documentation; replace verbose prose
with a short one-line note only if it adds non-obvious context, otherwise delete
the comment lines above the test functions in snapshot_dep_ref/tests.rs so the
tests remain concise and still compile/run.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6075d957-bb16-4a38-a97a-ffbdb134f65c

📥 Commits

Reviewing files that changed from the base of the PR and between 4b25d60 and bd2b2f2.

📒 Files selected for processing (14)
  • pacquet/crates/lockfile/src/load_lockfile/tests.rs
  • pacquet/crates/lockfile/src/resolved_dependency.rs
  • pacquet/crates/lockfile/src/snapshot_dep_ref.rs
  • pacquet/crates/lockfile/src/snapshot_dep_ref/tests.rs
  • pacquet/crates/package-manager/src/build_sequence.rs
  • pacquet/crates/package-manager/src/create_symlink_layout.rs
  • pacquet/crates/package-manager/src/current_lockfile.rs
  • pacquet/crates/package-manager/src/deps_graph.rs
  • pacquet/crates/package-manager/src/hoist.rs
  • pacquet/crates/package-manager/src/hoisted_dep_graph.rs
  • pacquet/crates/package-manager/src/install_frozen_lockfile.rs
  • pacquet/crates/package-manager/src/link_bins.rs
  • pacquet/crates/package-manager/src/virtual_store_layout.rs
  • pacquet/crates/real-hoist/src/lib.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
pacquet/**/*.rs

📄 CodeRabbit inference engine (pacquet/AGENTS.md)

pacquet/**/*.rs: When porting a function that fires pnpm:<channel> events through globalLogger, logger.debug(), or streamParser.write(), mirror the call site, payload, and ordering so the reporter parses pacquet's NDJSON the same way it parses pnpm's.
Declare a newtype wrapper for branded string types. Do not collapse the brand into a plain String or &str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only via TryFrom<String> and/or FromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallible From<String> (and From<&str> when convenient).
If upstream occasionally constructs without validation, expose from_str_unchecked as an escape hatch alongside the validating constructor.
Match upstream serde behavior for branded types that cross JSON, YAML, or INI boundaries. Use #[serde(try_from = "String")] for deserialization and #[serde(into = "String")] for serialization.
Use #[derive(derive_more::From)] and #[derive(derive_more::Into)] for mechanical conversion impls. Fall back to manual impl only when conversion needs custom logic.
String-literal unions should become enums, not newtype wrappers. Model closed sets of valid string values as enums.
Template literal types should be treated as branded strings with validation discipline from rules 2-5.
Choose owned vs. borrowed parameters to minimize copies. Widen to the most encompassing type (&Path over &PathBuf, &str over &String) when it doesn't force extra copies.
Prefer Arc::clone(&x) / Rc::clone(&x) over x.clone() for reference-counted types, so the cost is visible at the call site.
Follow Rust API Guidelines for naming conventions.
Do not use star imports inside module bodies. Write use super::{Foo, bar} instead of use super::*;. Two forms stay allowed: external-crate preludes like use rayon::prelude::*; and root-of-module re-...

Files:

  • pacquet/crates/package-manager/src/deps_graph.rs
  • pacquet/crates/package-manager/src/create_symlink_layout.rs
  • pacquet/crates/real-hoist/src/lib.rs
  • pacquet/crates/package-manager/src/virtual_store_layout.rs
  • pacquet/crates/lockfile/src/resolved_dependency.rs
  • pacquet/crates/package-manager/src/build_sequence.rs
  • pacquet/crates/package-manager/src/hoisted_dep_graph.rs
  • pacquet/crates/lockfile/src/load_lockfile/tests.rs
  • pacquet/crates/package-manager/src/link_bins.rs
  • pacquet/crates/lockfile/src/snapshot_dep_ref/tests.rs
  • pacquet/crates/package-manager/src/hoist.rs
  • pacquet/crates/lockfile/src/snapshot_dep_ref.rs
  • pacquet/crates/package-manager/src/install_frozen_lockfile.rs
  • pacquet/crates/package-manager/src/current_lockfile.rs
🧠 Learnings (1)
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.

Applied to files:

  • pacquet/crates/package-manager/src/deps_graph.rs
  • pacquet/crates/package-manager/src/create_symlink_layout.rs
  • pacquet/crates/real-hoist/src/lib.rs
  • pacquet/crates/package-manager/src/virtual_store_layout.rs
  • pacquet/crates/lockfile/src/resolved_dependency.rs
  • pacquet/crates/package-manager/src/build_sequence.rs
  • pacquet/crates/package-manager/src/hoisted_dep_graph.rs
  • pacquet/crates/lockfile/src/load_lockfile/tests.rs
  • pacquet/crates/package-manager/src/link_bins.rs
  • pacquet/crates/lockfile/src/snapshot_dep_ref/tests.rs
  • pacquet/crates/package-manager/src/hoist.rs
  • pacquet/crates/lockfile/src/snapshot_dep_ref.rs
  • pacquet/crates/package-manager/src/install_frozen_lockfile.rs
  • pacquet/crates/package-manager/src/current_lockfile.rs
🔇 Additional comments (14)
pacquet/crates/lockfile/src/snapshot_dep_ref.rs (1)

9-10: LGTM!

Also applies to: 30-35, 43-47, 55-55, 63-66, 69-69, 71-73, 78-80, 82-85, 88-95, 104-104, 136-138

pacquet/crates/lockfile/src/snapshot_dep_ref/tests.rs (1)

50-51: LGTM!

Also applies to: 57-58, 66-69, 79-80, 93-93, 107-111, 133-134, 138-138, 141-141, 143-144

pacquet/crates/lockfile/src/load_lockfile/tests.rs (1)

1-3: LGTM!

Also applies to: 162-167, 169-225

pacquet/crates/lockfile/src/resolved_dependency.rs (1)

51-54: LGTM!

pacquet/crates/package-manager/src/create_symlink_layout.rs (1)

63-68: LGTM!

pacquet/crates/package-manager/src/link_bins.rs (1)

422-427: LGTM!

pacquet/crates/package-manager/src/install_frozen_lockfile.rs (1)

1337-1341: LGTM!

pacquet/crates/package-manager/src/build_sequence.rs (1)

105-107: LGTM!

pacquet/crates/package-manager/src/deps_graph.rs (1)

153-158: LGTM!

pacquet/crates/package-manager/src/hoist.rs (1)

89-93: LGTM!

pacquet/crates/package-manager/src/hoisted_dep_graph.rs (1)

819-825: LGTM!

pacquet/crates/package-manager/src/current_lockfile.rs (1)

214-217: LGTM!

pacquet/crates/package-manager/src/virtual_store_layout.rs (1)

345-348: LGTM!

pacquet/crates/real-hoist/src/lib.rs (1)

504-511: LGTM!

@github-actions

github-actions Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

Micro-Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.03      8.0±0.35ms   544.6 KB/sec    1.00      7.7±0.15ms   563.3 KB/sec

@codecov-commenter

codecov-commenter commented May 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 76.47059% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.42%. Comparing base (998ea4c) to head (e1888c8).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
pacquet/crates/lockfile/src/snapshot_dep_ref.rs 94.11% 1 Missing ⚠️
...cquet/crates/package-manager/src/build_sequence.rs 50.00% 1 Missing ⚠️
...rates/package-manager/src/create_symlink_layout.rs 50.00% 1 Missing ⚠️
pacquet/crates/package-manager/src/deps_graph.rs 50.00% 1 Missing ⚠️
...et/crates/package-manager/src/hoisted_dep_graph.rs 50.00% 1 Missing ⚠️
...tes/package-manager/src/install_frozen_lockfile.rs 50.00% 1 Missing ⚠️
...crates/package-manager/src/virtual_store_layout.rs 50.00% 1 Missing ⚠️
pacquet/crates/real-hoist/src/lib.rs 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11788      +/-   ##
==========================================
- Coverage   89.65%   89.42%   -0.23%     
==========================================
  Files         174      175       +1     
  Lines       20836    21130     +294     
==========================================
+ Hits        18680    18895     +215     
- Misses       2156     2235      +79     

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

An injected workspace package's snapshot can carry a `link:<path>`
dependency value when the dep is a workspace sibling. Pnpm accepts
the shape — `refToRelative` short-circuits to `null` for `link:` —
but pacquet's `SnapshotDepRef` only handled the plain/alias shapes
and rejected `link:` at parse time.

Add a `SnapshotDepRef::Link(String)` variant mirroring
`ImporterDepVersion::Link`, return `None` from `resolve` / `ver_peer`
for it, and skip it at every consumer (matching upstream's
`if (childDepPath)` guards in `getChildren` /
`lockfileDepsToGraphChildren`).

Fixes #11775.
@github-actions

Copy link
Copy Markdown
Contributor

Integrated-Benchmark Report (Linux)

Scenario: Frozen Lockfile

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 2.371 ± 0.063 2.286 2.460 1.01 ± 0.03
pacquet@main 2.353 ± 0.043 2.291 2.421 1.00
pnpm 4.585 ± 0.065 4.482 4.687 1.95 ± 0.05
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 2.3714849058,
      "stddev": 0.06318448235420429,
      "median": 2.377216333,
      "user": 2.82684116,
      "system": 3.6287748800000004,
      "min": 2.286037673,
      "max": 2.460364036,
      "times": [
        2.460364036,
        2.286037673,
        2.357772674,
        2.457165566,
        2.287569577,
        2.396659992,
        2.321036271,
        2.4015681260000004,
        2.40549408,
        2.341181063
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 2.353438949,
      "stddev": 0.04283376132443959,
      "median": 2.349442293,
      "user": 2.8139023599999997,
      "system": 3.6150974799999993,
      "min": 2.2909715100000003,
      "max": 2.421483283,
      "times": [
        2.409406403,
        2.3521952660000003,
        2.3466893200000003,
        2.2909715100000003,
        2.3747639900000004,
        2.3795497780000003,
        2.308477648,
        2.3193671900000004,
        2.421483283,
        2.3314851020000003
      ]
    },
    {
      "command": "pnpm",
      "mean": 4.5850333295,
      "stddev": 0.06526931777984148,
      "median": 4.581607828999999,
      "user": 7.73848876,
      "system": 4.08555418,
      "min": 4.4818831139999995,
      "max": 4.687392303999999,
      "times": [
        4.562423975,
        4.501060642,
        4.574567882999999,
        4.4818831139999995,
        4.6795869759999995,
        4.687392303999999,
        4.588647775,
        4.598475779999999,
        4.5740450059999995,
        4.60224984
      ]
    }
  ]
}

Scenario: Frozen Lockfile (Hot Cache)

Command Mean [ms] Min [ms] Max [ms] Relative
pacquet@HEAD 726.5 ± 34.5 704.6 820.1 1.00 ± 0.07
pacquet@main 723.8 ± 36.0 689.5 784.9 1.00
pnpm 2512.5 ± 65.9 2391.6 2655.8 3.47 ± 0.20
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 0.72651986988,
      "stddev": 0.03447938767555693,
      "median": 0.71248237278,
      "user": 0.39451067999999995,
      "system": 1.6033434200000003,
      "min": 0.7046080212800001,
      "max": 0.8200909042800001,
      "times": [
        0.8200909042800001,
        0.7090938092800001,
        0.71231540228,
        0.7239300862800001,
        0.7046080212800001,
        0.73452960028,
        0.7057334562800001,
        0.71100494128,
        0.7312431342800001,
        0.7126493432800001
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 0.72382535678,
      "stddev": 0.03604301388749812,
      "median": 0.7042564557800001,
      "user": 0.38482367999999995,
      "system": 1.61310632,
      "min": 0.6895046962800001,
      "max": 0.78488033428,
      "times": [
        0.73894456228,
        0.74007233528,
        0.69968817528,
        0.6895046962800001,
        0.6972309412800001,
        0.78488033428,
        0.69651085828,
        0.70497657428,
        0.7035363372800001,
        0.7829087532800001
      ]
    },
    {
      "command": "pnpm",
      "mean": 2.51246736448,
      "stddev": 0.06591101429431272,
      "median": 2.5134972497800003,
      "user": 2.986440880000001,
      "system": 2.18897112,
      "min": 2.39155676528,
      "max": 2.65575032928,
      "times": [
        2.51592506328,
        2.50554233228,
        2.53829768628,
        2.52226891628,
        2.51106943628,
        2.5304605862800003,
        2.39155676528,
        2.48852413928,
        2.46527839028,
        2.65575032928
      ]
    }
  ]
}

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pacquet/crates/lockfile/src/snapshot_dep_ref.rs`:
- Around line 52-56: Introduce a newtype LinkTarget and replace the raw String
in the SnapshotDepRef::Link variant with Link(LinkTarget); add an infallible
From<String> implementation for LinkTarget and update all pattern matches and
accessors (e.g., any existing as_link_target() helpers) to accept/return
LinkTarget instead of String, then update serde (Serialize/Deserialize) handling
to serialize the inner string of LinkTarget; apply the same substitutions in
resolved_dependency.rs and tests (there are ~16 call sites) so all construction,
matching, and (de)serialization use LinkTarget rather than raw String.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 86007580-b378-4052-aa0d-c3ccd85db969

📥 Commits

Reviewing files that changed from the base of the PR and between bd2b2f2 and d62284a.

📒 Files selected for processing (14)
  • pacquet/crates/lockfile/src/load_lockfile/tests.rs
  • pacquet/crates/lockfile/src/resolved_dependency.rs
  • pacquet/crates/lockfile/src/snapshot_dep_ref.rs
  • pacquet/crates/lockfile/src/snapshot_dep_ref/tests.rs
  • pacquet/crates/package-manager/src/build_sequence.rs
  • pacquet/crates/package-manager/src/create_symlink_layout.rs
  • pacquet/crates/package-manager/src/current_lockfile.rs
  • pacquet/crates/package-manager/src/deps_graph.rs
  • pacquet/crates/package-manager/src/hoist.rs
  • pacquet/crates/package-manager/src/hoisted_dep_graph.rs
  • pacquet/crates/package-manager/src/install_frozen_lockfile.rs
  • pacquet/crates/package-manager/src/link_bins.rs
  • pacquet/crates/package-manager/src/virtual_store_layout.rs
  • pacquet/crates/real-hoist/src/lib.rs
✅ Files skipped from review due to trivial changes (1)
  • pacquet/crates/lockfile/src/resolved_dependency.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
pacquet/**/*.rs

📄 CodeRabbit inference engine (pacquet/AGENTS.md)

pacquet/**/*.rs: When porting a function that fires pnpm:<channel> events through globalLogger, logger.debug(), or streamParser.write(), mirror the call site, payload, and ordering so the reporter parses pacquet's NDJSON the same way it parses pnpm's.
Declare a newtype wrapper for branded string types. Do not collapse the brand into a plain String or &str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only via TryFrom<String> and/or FromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallible From<String> (and From<&str> when convenient).
If upstream occasionally constructs without validation, expose from_str_unchecked as an escape hatch alongside the validating constructor.
Match upstream serde behavior for branded types that cross JSON, YAML, or INI boundaries. Use #[serde(try_from = "String")] for deserialization and #[serde(into = "String")] for serialization.
Use #[derive(derive_more::From)] and #[derive(derive_more::Into)] for mechanical conversion impls. Fall back to manual impl only when conversion needs custom logic.
String-literal unions should become enums, not newtype wrappers. Model closed sets of valid string values as enums.
Template literal types should be treated as branded strings with validation discipline from rules 2-5.
Choose owned vs. borrowed parameters to minimize copies. Widen to the most encompassing type (&Path over &PathBuf, &str over &String) when it doesn't force extra copies.
Prefer Arc::clone(&x) / Rc::clone(&x) over x.clone() for reference-counted types, so the cost is visible at the call site.
Follow Rust API Guidelines for naming conventions.
Do not use star imports inside module bodies. Write use super::{Foo, bar} instead of use super::*;. Two forms stay allowed: external-crate preludes like use rayon::prelude::*; and root-of-module re-...

Files:

  • pacquet/crates/package-manager/src/install_frozen_lockfile.rs
  • pacquet/crates/package-manager/src/hoist.rs
  • pacquet/crates/package-manager/src/build_sequence.rs
  • pacquet/crates/package-manager/src/hoisted_dep_graph.rs
  • pacquet/crates/package-manager/src/create_symlink_layout.rs
  • pacquet/crates/lockfile/src/load_lockfile/tests.rs
  • pacquet/crates/real-hoist/src/lib.rs
  • pacquet/crates/package-manager/src/current_lockfile.rs
  • pacquet/crates/lockfile/src/snapshot_dep_ref/tests.rs
  • pacquet/crates/package-manager/src/link_bins.rs
  • pacquet/crates/package-manager/src/deps_graph.rs
  • pacquet/crates/package-manager/src/virtual_store_layout.rs
  • pacquet/crates/lockfile/src/snapshot_dep_ref.rs
🧠 Learnings (2)
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.

Applied to files:

  • pacquet/crates/package-manager/src/install_frozen_lockfile.rs
  • pacquet/crates/package-manager/src/hoist.rs
  • pacquet/crates/package-manager/src/build_sequence.rs
  • pacquet/crates/package-manager/src/hoisted_dep_graph.rs
  • pacquet/crates/package-manager/src/create_symlink_layout.rs
  • pacquet/crates/lockfile/src/load_lockfile/tests.rs
  • pacquet/crates/real-hoist/src/lib.rs
  • pacquet/crates/package-manager/src/current_lockfile.rs
  • pacquet/crates/lockfile/src/snapshot_dep_ref/tests.rs
  • pacquet/crates/package-manager/src/link_bins.rs
  • pacquet/crates/package-manager/src/deps_graph.rs
  • pacquet/crates/package-manager/src/virtual_store_layout.rs
  • pacquet/crates/lockfile/src/snapshot_dep_ref.rs
📚 Learning: 2026-05-20T23:07:58.444Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:07:58.444Z
Learning: When reviewing code in this pacquet Rust port, follow the upstream pnpm compatibility rule: only match pnpm’s behavior exactly. Do not propose review changes that intentionally deviate from pnpm’s documented/observed behavior, even if pnpm appears buggy. If you identify a real bug in pnpm behavior, the review should prioritize fixing it upstream in pnpm first, and avoid implementing a pnpm-behavior workaround here unless the same fix has already landed upstream.

Applied to files:

  • pacquet/crates/package-manager/src/install_frozen_lockfile.rs
  • pacquet/crates/package-manager/src/hoist.rs
  • pacquet/crates/package-manager/src/build_sequence.rs
  • pacquet/crates/package-manager/src/hoisted_dep_graph.rs
  • pacquet/crates/package-manager/src/create_symlink_layout.rs
  • pacquet/crates/lockfile/src/load_lockfile/tests.rs
  • pacquet/crates/real-hoist/src/lib.rs
  • pacquet/crates/package-manager/src/current_lockfile.rs
  • pacquet/crates/lockfile/src/snapshot_dep_ref/tests.rs
  • pacquet/crates/package-manager/src/link_bins.rs
  • pacquet/crates/package-manager/src/deps_graph.rs
  • pacquet/crates/package-manager/src/virtual_store_layout.rs
  • pacquet/crates/lockfile/src/snapshot_dep_ref.rs
🔇 Additional comments (13)
pacquet/crates/package-manager/src/build_sequence.rs (1)

105-107: LGTM!

pacquet/crates/package-manager/src/deps_graph.rs (1)

153-158: LGTM!

pacquet/crates/package-manager/src/hoist.rs (1)

89-94: LGTM!

pacquet/crates/package-manager/src/hoisted_dep_graph.rs (1)

819-825: LGTM!

pacquet/crates/package-manager/src/virtual_store_layout.rs (1)

345-348: LGTM!

pacquet/crates/real-hoist/src/lib.rs (1)

504-511: LGTM!

pacquet/crates/package-manager/src/current_lockfile.rs (1)

214-217: LGTM!

pacquet/crates/package-manager/src/create_symlink_layout.rs (1)

63-68: LGTM!

pacquet/crates/package-manager/src/link_bins.rs (1)

422-427: LGTM!

pacquet/crates/package-manager/src/install_frozen_lockfile.rs (1)

1337-1341: LGTM!

pacquet/crates/lockfile/src/snapshot_dep_ref.rs (1)

9-47: LGTM!

Also applies to: 63-73, 78-95, 104-105, 136-138

pacquet/crates/lockfile/src/snapshot_dep_ref/tests.rs (1)

50-51: LGTM!

Also applies to: 57-58, 61-69, 79-80, 93-97, 100-111, 133-145

pacquet/crates/lockfile/src/load_lockfile/tests.rs (1)

1-4: LGTM!

Also applies to: 163-218, 220-283

Comment thread pacquet/crates/lockfile/src/snapshot_dep_ref.rs
Drop test doc-comments that restated the assertion. The test name +
asserts already convey the intent; the upstream-parity reason lives
at the `SnapshotDepRef::resolve` / `Link` variant docs.
@zkochan zkochan merged commit d08e9ab into main May 20, 2026
28 checks passed
@zkochan zkochan deleted the fix/11775 branch May 20, 2026 23:30
@coderabbitai coderabbitai Bot mentioned this pull request Jun 22, 2026
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pacquet fails to parse lockfile snapshot dependency with link: workspace path

2 participants