This repository was archived by the owner on May 14, 2026. It is now read-only.
feat(lockfile): PkgIdWithPatchHash newtype, replace String in dep-graph nodes (#481)#504
Merged
Merged
Conversation
…raph nodes (#481) Port upstream's [`PkgIdWithPatchHash`](https://github.com/pnpm/pnpm/blob/94240bc046/core/types/src/misc.ts) branded string (`string & { __brand: 'PkgIdWithPatchHash' }`) as a non-validating newtype in `pacquet-lockfile`, matching `CLAUDE.md`'s rule 3 for non-validating brands: `From<String>` / `From<&str>` via `derive_more`, `#[serde(transparent)]` for wire-format identity with `String`, no validating constructor. Modeled on `pacquet_modules_yaml::DepPath` — the sibling brand from the same upstream `misc.ts` file under the same rules. Replaces the plain `String` field/parameter in the two pacquet consumers CodeRabbit flagged on #478: - `DependenciesGraphNode.pkg_id_with_patch_hash` in `package-manager/src/hoisted_dep_graph.rs`. - `create_full_pkg_id`'s first parameter and the `lockfile_to_dep_graph` local in `package-manager/src/virtual_store_layout.rs`. A workspace audit (`grep "pkg_id_with_patch_hash"`) turns up no other slots typed as plain `String`. Two new tests in `pkg_id_with_patch_hash.rs` pin the contract: a serde round-trip (`#[serde(transparent)]` keeps the on-disk shape a raw string) and the non-validating-construction property (empty string and `From<&str>`/`From<String>` both work).
📝 WalkthroughWalkthroughThis PR introduces ChangesPackage ID type branding
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #504 +/- ##
==========================================
+ Coverage 88.42% 88.74% +0.31%
==========================================
Files 121 124 +3
Lines 12969 13429 +460
==========================================
+ Hits 11468 11917 +449
- Misses 1501 1512 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Micro-Benchmark ResultsLinux |
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.59546451,
"stddev": 0.08521357358468006,
"median": 2.5838036852000004,
"user": 2.59937604,
"system": 3.5315286,
"min": 2.4156095492,
"max": 2.7499720612000003,
"times": [
2.5854470602000004,
2.5667965542,
2.5783204402,
2.7499720612000003,
2.5728725042000002,
2.6175064772,
2.4156095492,
2.5821603102000004,
2.6763489322000003,
2.6096112112000003
]
},
{
"command": "pacquet@main",
"mean": 2.5095154518999996,
"stddev": 0.05521738608724806,
"median": 2.5124818412000005,
"user": 2.6219407400000003,
"system": 3.5351824,
"min": 2.4378909922000003,
"max": 2.5913017122000004,
"times": [
2.4627018982,
2.4378909922000003,
2.5913017122000004,
2.5101144952000003,
2.4559551592,
2.5683628722000003,
2.5367041282000002,
2.4523841512,
2.5148491872000003,
2.5648899232000004
]
},
{
"command": "pnpm",
"mean": 5.770427225499999,
"stddev": 0.06521329680581212,
"median": 5.7575467377,
"user": 8.393038639999999,
"system": 4.2931431,
"min": 5.7034793792,
"max": 5.9320891082,
"times": [
5.7697508652,
5.7135171332,
5.8156451582,
5.7713406902,
5.7034793792,
5.9320891082,
5.7595381082,
5.7554789652,
5.7555553671999995,
5.7278774802
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.7478052871800001,
"stddev": 0.04445924023237604,
"median": 0.7359816316800001,
"user": 0.35847558,
"system": 1.61325462,
"min": 0.7106568171800001,
"max": 0.86392973718,
"times": [
0.86392973718,
0.76065505918,
0.7106568171800001,
0.74831608118,
0.71414919318,
0.72190323118,
0.72574923118,
0.7393410071800001,
0.73262225618,
0.7607302581800001
]
},
{
"command": "pacquet@main",
"mean": 0.7832404312800001,
"stddev": 0.08005377931417851,
"median": 0.74628283018,
"user": 0.38309227999999995,
"system": 1.6205512199999998,
"min": 0.70755546218,
"max": 0.9335729151800001,
"times": [
0.9335729151800001,
0.7957278561800001,
0.72778794618,
0.75181780718,
0.9029881941800001,
0.74074785318,
0.70755546218,
0.7137338291800001,
0.8246384371800001,
0.73383401218
]
},
{
"command": "pnpm",
"mean": 2.48428508778,
"stddev": 0.10070762859866679,
"median": 2.4350838146800005,
"user": 2.8516474799999996,
"system": 2.21591552,
"min": 2.3674462691800002,
"max": 2.6555425631800005,
"times": [
2.4298029651800004,
2.39294939518,
2.5117324451800003,
2.58639859818,
2.61224016918,
2.43702243818,
2.41657084318,
2.3674462691800002,
2.6555425631800005,
2.4331451911800004
]
}
]
} |
…lain text CI \`Doc\` job runs with \`RUSTDOCFLAGS=-D warnings\`, so the two \`[\`pacquet_modules_yaml::DepPath\`]\` intra-doc links in \`pkg_id_with_patch_hash.rs\` failed \`rustdoc::broken-intra-doc-links\`: \`pacquet-lockfile\` doesn't depend on \`pacquet-modules-yaml\` (the dependency would go the wrong way), so the rustdoc resolver can't follow the link. Switched both references to bare \`pacquet_modules_yaml::DepPath\` prose, with a note that the bare reference is intentional. The docstring still tells a reader where to find the sibling brand; it just stops resolving as a clickable link, which costs nothing practical since the type name is searchable in any IDE. Adding \`pacquet-modules-yaml\` as a dev-dep purely for a doc link would invert the natural crate ordering (lockfile is upstream of modules-yaml in the build graph) — rejected as a cosmetic fix that introduces a real architectural smell.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/lockfile/src/pkg_id_with_patch_hash.rs (1)
6-11: ⚡ Quick winUse
blob/mainfor upstream pnpm links in rustdoc.Line 6, Line 10, and Line 11 use commit-pinned URLs. Please switch these to
https://github.com/pnpm/pnpm/blob/main/...to avoid stale references in future ports.Suggested doc-link update
-/// [`PkgIdWithPatchHash`](https://github.com/pnpm/pnpm/blob/94240bc046/core/types/src/misc.ts) +/// [`PkgIdWithPatchHash`](https://github.com/pnpm/pnpm/blob/main/core/types/src/misc.ts) @@ -/// see [`createFullPkgId`](https://github.com/pnpm/pnpm/blob/94240bc046/deps/graph-hasher/src/index.ts#L248-L274) -/// and [`createPatchHash`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/index.ts). +/// see [`createFullPkgId`](https://github.com/pnpm/pnpm/blob/main/deps/graph-hasher/src/index.ts#L248-L274) +/// and [`createPatchHash`](https://github.com/pnpm/pnpm/blob/main/installing/deps-installer/src/install/index.ts).As per coding guidelines: "If reading on GitHub, open files from
https://github.com/pnpm/pnpm/blob/main/...... rather than from a permalinked SHA."🤖 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/lockfile/src/pkg_id_with_patch_hash.rs` around lines 6 - 11, The rustdoc contains commit-pinned GitHub links to pnpm; update those URLs to use the non-permalink branch path (replace the SHA-based blob links with https://github.com/pnpm/pnpm/blob/main/...) in the doc comment for PkgIdWithPatchHash and the referenced links to createFullPkgId and createPatchHash so the docs point to blob/main instead of the specific commit SHA.
🤖 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/lockfile/src/pkg_id_with_patch_hash.rs`:
- Around line 6-11: The rustdoc contains commit-pinned GitHub links to pnpm;
update those URLs to use the non-permalink branch path (replace the SHA-based
blob links with https://github.com/pnpm/pnpm/blob/main/...) in the doc comment
for PkgIdWithPatchHash and the referenced links to createFullPkgId and
createPatchHash so the docs point to blob/main instead of the specific commit
SHA.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 75308105-e9e1-4eb3-b783-b1693bfd5793
📒 Files selected for processing (1)
crates/lockfile/src/pkg_id_with_patch_hash.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). (6)
- GitHub Check: Code Coverage
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Run benchmark on ubuntu-latest
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Preserve existing method chains andpipe-traitchains; do not break them into intermediateletbindings unless there is a concrete justification such as a compilation failure, borrow checker rejection, meaningful performance improvement, or other technical necessity. Refactoring for style alone is not sufficient justification.
Choose owned vs. borrowed parameters to minimize copies; prefer borrowed types (&Pathover&PathBuf,&strover&String) when it does not force extra copies.
PreferArc::clone(&x)andRc::clone(&x)overx.clone()for reference-counted types to make the cost visible at the call site.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;for any glob whose target is a module you control. External-crate preludes (e.g.,use rayon::prelude::*;) and root-of-module re-exports (e.g.,pub use submodule::*;inlib.rs) are exceptions.
Follow Rust API Guidelines for naming, as documented in https://rust-lang.github.io/api-guidelines/naming.html.
Declare a newtype wrapper for any branded string type being ported from TypeScript pnpm. Do not collapse the brand into a plainStringor&str; give the type its own struct so misuse is a type error.
When porting branded string types where upstream TypeScript always validates before construction, validate in the Rust port too. Construct the wrapper only viaTryFrom<String>and/orFromStr; do not provide an infallible public constructor that takes an arbitrary string.
For branded string types where upstream TypeScript never validates (used purely for type-safety to prevent confusion between string slots), expose an infallibleFrom<String>andFrom<&str>constructor in the Rust wrapper.
When upstream TypeScript occasionally constructs a branded type without validation (via bareasassertion), add afrom_str_unchecked(or similarly named) constructor on the Rust side. Keep the validating constructor as well; `from_str_u...
Files:
crates/lockfile/src/pkg_id_with_patch_hash.rs
🧠 Learnings (1)
📚 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/lockfile/src/pkg_id_with_patch_hash.rs
🔇 Additional comments (2)
crates/lockfile/src/pkg_id_with_patch_hash.rs (2)
31-55: LGTM!
57-92: LGTM!
This was referenced May 13, 2026
zkochan
added a commit
that referenced
this pull request
May 13, 2026
…508) The Slice 5 linker (#505) was written against `String` for `pkg_id_with_patch_hash` and merged onto a main that had already switched the underlying `DependenciesGraphNode` field to the `PkgIdWithPatchHash` newtype (#504). The merge auto-resolved without flagging the type incompatibility — `cargo check` on main now errors with: error[E0277]: the trait bound `String: Borrow<PkgIdWithPatchHash>` is not satisfied error[E0308]: mismatched types expected `String`, found `PkgIdWithPatchHash` Switch the two slice-5-introduced surfaces to match the newtype: - `CasPathsByPkgId = HashMap<PkgIdWithPatchHash, HashMap<String, PathBuf>>` — the per-package CAS index now keys on the brand the graph node carries, matching the post-#504 invariant across the workspace. - `LinkHoistedModulesError::MissingCasPaths.pkg_id_with_patch_hash: PkgIdWithPatchHash` — same type the graph node has, so the error round-trips the value without `.to_string()`. Tests updated to construct `PkgIdWithPatchHash::from(...)` keys/fields. All 8 linker tests pass on the fixed branch. This unblocks main building again.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #481. Ports upstream's
PkgIdWithPatchHashbranded string (string & { __brand: 'PkgIdWithPatchHash' }) as a non-validating newtype inpacquet-lockfile, replacing the plainStringfield/parameter at the two consumer sites CodeRabbit flagged on #478.Newtype
crates/lockfile/src/pkg_id_with_patch_hash.rs— modelled onpacquet_modules_yaml::DepPath(the sibling brand from the same upstreammisc.tsfile under the sameCLAUDE.mdrule 3 contract):Plus manual
From<&str>(so call sites taking string literals don't pay an intermediateto_string()) andfmt::Display(so the existingformat!("{pkg_id_with_patch_hash}:{integrity}")increate_full_pkg_idkeeps working unchanged). No validating constructor — upstream constructs the value with bareascasts everywhere, no checks.Consumer migrations
crates/package-manager/src/hoisted_dep_graph.rs:DependenciesGraphNode.pkg_id_with_patch_hashnowPkgIdWithPatchHash. Both the production write site (fill_childrenwalker) and the test fixture construct viaPkgIdWithPatchHash::from(...).crates/package-manager/src/virtual_store_layout.rs:create_full_pkg_id's first parameter and thelockfile_to_dep_graphlocal bothPkgIdWithPatchHash. TheDisplayimpl makes theformat!("{...}:{integrity}")call site work unchanged.Workspace audit
grep "pkg_id_with_patch_hash"shows no other slots typed as plainString— all remaining hits are either the local variable (now typed), comments describing the value's role, or the module declaration. Reserved for follow-ups: there's nopacquet_modules_yaml::Modulesuse of this brand today (upstream stores it insidedependencies-hierarchy/.../HoistedDependenciesonly, not in.modules.yaml), so the wire-format wins from#[serde(transparent)]are pre-paid.Test plan
cargo nextest run -p pacquet-lockfile -p pacquet-package-manager— 315 + 2 tests pass (+2 new inpkg_id_with_patch_hash::tests: serde round-trip matches plain string; non-validatingFrom<String>/From<&str>accept empty and non-empty inputs).just ready— workspace clean.just dylint— perfectionist lints clean.Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit