feat(lockfile): emit patchedDependencies block in pacquet lockfile#12281
Merged
Conversation
📝 WalkthroughWalkthroughConfig resolves patch paths and computes SHA-256 digests via Config::patched_dependency_hashes; Lockfile gains a top-level patchedDependencies BTreeMap; plumbing threads these hashes into lockfile construction, freshness checks, optimistic fast paths, and install flows; tests and fixtures updated accordingly. ChangesPatched Dependencies Hashing and Lockfile Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
32b8961 to
c8244f7
Compare
Contributor
Micro-Benchmark ResultsLinux |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12281 +/- ##
=======================================
Coverage 87.54% 87.55%
=======================================
Files 280 280
Lines 33476 33540 +64
=======================================
+ Hits 29307 29365 +58
- Misses 4169 4175 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code Review by Qodo
1. Fast-path misses patch edits
|
Contributor
Integrated-Benchmark Report (Linux)Each scenario reports direct installs and pnpr installs. Bencher consumes pacquet@HEAD and pnpr@HEAD. Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 9.97412217692,
"stddev": 0.19122886089543223,
"median": 9.89024299242,
"user": 3.0562098399999997,
"system": 3.2469278599999996,
"min": 9.81712069642,
"max": 10.33304234842,
"times": [
10.02066742442,
9.96216677842,
10.33304234842,
10.299377419419999,
9.90238017442,
9.87353835842,
9.82980909342,
9.81712069642,
9.82501366542,
9.87810581042
]
},
{
"command": "pacquet@main",
"mean": 9.930094471919997,
"stddev": 0.13508613739909,
"median": 9.891884136919998,
"user": 3.0833564399999993,
"system": 3.31841206,
"min": 9.81479780642,
"max": 10.277153886419999,
"times": [
9.85122031842,
9.84499596742,
9.925373879419999,
9.81479780642,
9.89610323342,
10.03054823742,
9.88884999342,
9.87698311642,
10.277153886419999,
9.894918280419999
]
},
{
"command": "pnpr@HEAD",
"mean": 5.38302285542,
"stddev": 0.12855450331427387,
"median": 5.33106323192,
"user": 2.5112710399999996,
"system": 2.96809186,
"min": 5.28086207742,
"max": 5.72123388242,
"times": [
5.40996726042,
5.35303376742,
5.32893361242,
5.72123388242,
5.333192851420001,
5.32570633942,
5.31643308042,
5.44805680242,
5.28086207742,
5.31280888042
]
},
{
"command": "pnpr@main",
"mean": 5.40675920852,
"stddev": 0.13735402194220353,
"median": 5.34892380242,
"user": 2.48444954,
"system": 2.9621580599999997,
"min": 5.31360033042,
"max": 5.73044855942,
"times": [
5.31892165942,
5.35704221942,
5.31360033042,
5.34163177942,
5.3562158254200005,
5.574811164420001,
5.41040934842,
5.333251858420001,
5.33125934042,
5.73044855942
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6565091374,
"stddev": 0.010988727521776008,
"median": 0.65687256,
"user": 0.38811531999999993,
"system": 1.3259450199999998,
"min": 0.6401165305000001,
"max": 0.6785752755000001,
"times": [
0.6785752755000001,
0.6570444755,
0.6567006445000001,
0.6584717875,
0.6401165305000001,
0.6658185755,
0.6611865955,
0.6436224755000001,
0.6498313615000001,
0.6537236525000001
]
},
{
"command": "pacquet@main",
"mean": 0.6758314305,
"stddev": 0.026712482083816325,
"median": 0.6688266140000001,
"user": 0.39228502,
"system": 1.33436192,
"min": 0.6495445715,
"max": 0.7295363275000001,
"times": [
0.6869546035,
0.7295363275000001,
0.7090356795,
0.6495445715,
0.6532599275000001,
0.6556282695000001,
0.6764849815,
0.6611682465000001,
0.6820540145,
0.6546476835
]
},
{
"command": "pnpr@HEAD",
"mean": 0.7781698551,
"stddev": 0.09528657947976128,
"median": 0.7501312535,
"user": 0.39609861999999996,
"system": 1.3165503199999997,
"min": 0.7194782605000001,
"max": 1.0426208305,
"times": [
0.7523916495,
0.7478708575,
0.7694881235000001,
0.7424034885,
0.7337739915,
0.7194782605000001,
1.0426208305,
0.7927057505,
0.7267586505000001,
0.7542069485
]
},
{
"command": "pnpr@main",
"mean": 0.7719287382,
"stddev": 0.04377163502586479,
"median": 0.7581983085000001,
"user": 0.38821332,
"system": 1.3165834200000002,
"min": 0.7287439845,
"max": 0.8767625865,
"times": [
0.7287439845,
0.7934248945000001,
0.8767625865,
0.7637436965000001,
0.7501290555000001,
0.7402394545000001,
0.7845824325,
0.7939838985000001,
0.7350244585000001,
0.7526529205000001
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 9.227795601939999,
"stddev": 0.0392318016853357,
"median": 9.21751972354,
"user": 3.5917806,
"system": 3.27838048,
"min": 9.17483847804,
"max": 9.30446261104,
"times": [
9.182860461039999,
9.30446261104,
9.21645935304,
9.21701108704,
9.24462781804,
9.21802836004,
9.23499329204,
9.27437232204,
9.17483847804,
9.21030223704
]
},
{
"command": "pacquet@main",
"mean": 9.259989389540001,
"stddev": 0.04005112972133877,
"median": 9.25446358154,
"user": 3.6714076999999996,
"system": 3.323112579999999,
"min": 9.20377104104,
"max": 9.331754071039999,
"times": [
9.331754071039999,
9.24785498704,
9.261072176039999,
9.21228654904,
9.24448965404,
9.26504631404,
9.27269634504,
9.245556495039999,
9.20377104104,
9.31536626304
]
},
{
"command": "pnpr@HEAD",
"mean": 5.09282045224,
"stddev": 0.03253103977183357,
"median": 5.090568901539999,
"user": 2.3624913,
"system": 2.860212379999999,
"min": 5.040061586039999,
"max": 5.15453099104,
"times": [
5.11322254904,
5.098778536039999,
5.060063621039999,
5.0859189450399995,
5.09475005604,
5.07198576604,
5.040061586039999,
5.08638774704,
5.12250472504,
5.15453099104
]
},
{
"command": "pnpr@main",
"mean": 5.14899734634,
"stddev": 0.157044569699118,
"median": 5.088436894039999,
"user": 2.3187606,
"system": 2.881109379999999,
"min": 5.03029194104,
"max": 5.5287680320399994,
"times": [
5.11791554604,
5.5287680320399994,
5.083954290039999,
5.06502205104,
5.09401407904,
5.09291949804,
5.03029194104,
5.33180039404,
5.06377998504,
5.08150764704
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.4136930228600002,
"stddev": 0.017994889234982908,
"median": 1.41121714596,
"user": 1.5722916599999999,
"system": 1.76230298,
"min": 1.39162775546,
"max": 1.44855055646,
"times": [
1.44855055646,
1.42610140446,
1.4216784254600001,
1.39162775546,
1.39821977646,
1.40858674446,
1.41384754746,
1.4014694754600001,
1.3964006524600001,
1.43044789046
]
},
{
"command": "pacquet@main",
"mean": 1.42771714376,
"stddev": 0.015810408441663322,
"median": 1.42747882446,
"user": 1.56412496,
"system": 1.8130688799999999,
"min": 1.40858857446,
"max": 1.45300176546,
"times": [
1.42626879646,
1.45054840746,
1.40858857446,
1.42877011646,
1.41266585146,
1.43817670346,
1.40925843546,
1.45300176546,
1.42868885246,
1.42120393446
]
},
{
"command": "pnpr@HEAD",
"mean": 0.6704852725600001,
"stddev": 0.013208644451633764,
"median": 0.66806564846,
"user": 0.34611896,
"system": 1.25925788,
"min": 0.6585060114600001,
"max": 0.70524248046,
"times": [
0.70524248046,
0.6676570244600001,
0.6585060114600001,
0.67227541546,
0.6619455874600001,
0.6690466404600001,
0.66504201446,
0.6750211634600001,
0.66847427246,
0.6616421154600001
]
},
{
"command": "pnpr@main",
"mean": 0.68778437326,
"stddev": 0.06440367503159798,
"median": 0.66734314896,
"user": 0.34594006,
"system": 1.2682856799999997,
"min": 0.65621164546,
"max": 0.8694590174600001,
"times": [
0.67710093646,
0.6686736454600001,
0.66791180746,
0.8694590174600001,
0.6650041184600001,
0.65621164546,
0.66677449046,
0.6601421644600001,
0.6857063764600001,
0.66085953046
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 4.994590336919999,
"stddev": 0.027915958970823475,
"median": 5.00115034122,
"user": 1.7729421000000003,
"system": 1.9021667800000004,
"min": 4.93889329372,
"max": 5.026225053719999,
"times": [
5.021929494719999,
4.986101284719999,
4.9845258677199995,
4.95980292672,
5.01116636872,
5.026225053719999,
5.01495839672,
4.99970986372,
5.00259081872,
4.93889329372
]
},
{
"command": "pacquet@main",
"mean": 5.021765816619999,
"stddev": 0.0369224457604047,
"median": 5.00833668872,
"user": 1.7803071,
"system": 1.91898788,
"min": 4.9884126347199995,
"max": 5.093919155719999,
"times": [
4.99410167572,
4.99672051372,
4.9886538077199996,
4.99853325372,
5.018140123719999,
5.02751527372,
5.093919155719999,
4.9884126347199995,
5.039171359719999,
5.0724903677199995
]
},
{
"command": "pnpr@HEAD",
"mean": 0.6746661433200001,
"stddev": 0.03398111774761516,
"median": 0.6663847457200001,
"user": 0.3472266999999999,
"system": 1.2563653800000003,
"min": 0.64012304072,
"max": 0.7660393217200001,
"times": [
0.7660393217200001,
0.6659378717200001,
0.6757109037200001,
0.66683161972,
0.65562074272,
0.6814573767200001,
0.6637750707200001,
0.66866735472,
0.64012304072,
0.6624981307200001
]
},
{
"command": "pnpr@main",
"mean": 0.6790304167200001,
"stddev": 0.024763683208613125,
"median": 0.6726585782200001,
"user": 0.34082409999999996,
"system": 1.27460178,
"min": 0.65571603272,
"max": 0.7268504597200001,
"times": [
0.6779450597200001,
0.6716171267200001,
0.7268504597200001,
0.66269351472,
0.6737000297200001,
0.65619845872,
0.65571603272,
0.7187724547200001,
0.66409439672,
0.68271663372
]
}
]
} |
Contributor
|
Bencher Report| Branch | pr/12281 |
| Testbed | pacquet |
🚨 1 Alert
| Benchmark | Measure Units | View | Benchmark Result (Result Δ%) | Upper Boundary (Limit %) |
|---|---|---|---|---|
| isolated-linker.fresh-install.cold-cache.cold-store | Latency seconds (s) | 📈 plot 🚷 threshold 🚨 alert (🔔) | 9.23 s(+48.55%)Baseline: 6.21 s | 7.45 s (123.79%) |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| isolated-linker.fresh-install.cold-cache.cold-store | 📈 view plot 🚷 view threshold 🚨 view alert (🔔) | 9,227.80 ms(+48.55%)Baseline: 6,212.05 ms | 7,454.47 ms (123.79%) |
| isolated-linker.fresh-install.cold-cache.hot-store | 📈 view plot 🚷 view threshold | 4,994.59 ms(-0.33%)Baseline: 5,011.36 ms | 6,013.63 ms (83.05%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,413.69 ms(+1.46%)Baseline: 1,393.41 ms | 1,672.09 ms (84.55%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 9,974.12 ms(+18.04%)Baseline: 8,450.06 ms | 10,140.08 ms (98.36%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 656.51 ms(+0.72%)Baseline: 651.85 ms | 782.22 ms (83.93%) |
|
Code review by qodo was updated up to the latest commit 65e5afb |
65e5afb to
ffaad73
Compare
|
Code review by qodo was updated up to the latest commit ffaad73 |
pacquet resolved and hashed patches for the depPath `(patch_hash=...)` suffix and at build time, but never wrote the top-level `patchedDependencies:` block into `pnpm-lock.yaml`, so a `pacquet install --lockfile-only` diverged from pnpm (issue item 6 of #12266). Add a `patched_dependencies` field to the `Lockfile` struct in its `sortLockfileKeys` slot (between `pnpmfileChecksum` and `importers`), populated via a new `Config::patched_dependency_hashes()` that ports pnpm's `calcPatchHashes(opts.patchedDependencies)`: resolve each patch path against the workspace dir and hash it, keeping the user's verbatim keys so a bare `foo` and `foo@*` stay separate lockfile keys rather than collapsing into one group bucket. The hashes are computed once per install and threaded through `GraphToLockfileOptions`; the current lockfile (`lock.yaml`) carries them through.
Now that the lockfile records `patchedDependencies` hashes, the frozen-lockfile freshness gate must reject an install when those hashes drift — otherwise editing a patch file would not invalidate the lockfile even though the patch hash participates in `(patch_hash=...)` depPath identity. Port pnpm's `getOutdatedLockfileSetting` patchedDependencies check: `check_lockfile_settings` now takes the current install's `patched_dependency_hashes()` and compares it (order-insensitively) against `lockfile.patched_dependencies`, surfacing a new `StalenessReason::PatchedDependenciesChanged` between the `ignoredOptionalDependencies` and settings checks, matching upstream's order. The pnpr fast-path bails when patches are configured. Addresses review feedback on the patchedDependencies-block PR.
…all fast path The optimistic repeat-install fast path skipped the whole pipeline before `check_lockfile_freshness` ran, and its workspace-state comparison only checks the `patchedDependencies` key→path map. A patch file edited in place (same config entry, new contents) therefore slipped through: `node_modules` and the lockfile's recorded patch hash could stay stale behind an "Already up to date". Port the patch-file branch of pnpm's `patchesOrHooksAreModified`: a configured patch whose mtime is newer than the workspace state's `lastValidatedTimestamp` invalidates the fast path, checked before the manifest-mtime exit so the patch reason wins. The pnpmfile branch and the `assertWantedLockfileUpToDate` re-verification remain unported. Addresses review feedback on the patchedDependencies-block PR.
ffaad73 to
a88cc77
Compare
|
Code review by qodo was updated up to the latest commit a88cc77 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pacquet/crates/package-manager/src/optimistic_repeat_install.rs (1)
452-488:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDoc comment for
manifests_unchanged_sinceis separated from its function.The doc comment at lines 452-457 describes statting
package.jsonfiles and referencesstatManifestFile, which clearly documentsmanifests_unchanged_since(line 490+), notpatches_modified_since. Inserting the new function between the doc comment and the function it documents breaks Rust's doc comment attachment rules — doc comments must immediately precede the item they document.📝 Proposed fix
Move the
manifests_unchanged_sincedoc comment to immediately precede its function:-/// Stat every project's `package.json` and check that no mtime is -/// newer than `cutoff_ms`. Any stat failure is treated as "can't -/// prove freshness, fall through" — matching pnpm's -/// [`statManifestFile`](https://github.com/pnpm/pnpm/blob/cc4ff817aa/deps/status/src/statManifestFile.ts) -/// behavior on missing files (it throws, which `checkDepsStatus` -/// catches via the outer `try`). /// Whether any configured patch file's mtime is newer than the last /// validation. Mirrors the patch branch of upstream's /// [`patchesOrHooksAreModified`](https://github.com/pnpm/pnpm/blob/cc4ff817aa/deps/status/src/checkDepsStatus.ts#L604-L613): /// `allPatchStats.some(patch => patch && patch.mtime > lastValidatedTimestamp)`. /// A patch that can't be stat'd is treated as not-modified — pnpm's /// `safeStat` returns null and the `patch &&` guard drops it, leaving a /// genuinely missing patch to surface on the full install path. Patch /// paths are resolved against `workspace_root` (the `pnpm-workspace.yaml` /// dir, where `patchedDependencies` is declared), matching how /// [`Config::patched_dependency_hashes`] resolves them. fn patches_modified_since(workspace_root: &Path, config: &Config, cutoff_ms: i64) -> bool { // ... body ... } +/// Stat every project's `package.json` and check that no mtime is +/// newer than `cutoff_ms`. Any stat failure is treated as "can't +/// prove freshness, fall through" — matching pnpm's +/// [`statManifestFile`](https://github.com/pnpm/pnpm/blob/cc4ff817aa/deps/status/src/statManifestFile.ts) +/// behavior on missing files (it throws, which `checkDepsStatus` +/// catches via the outer `try`). fn manifests_unchanged_since( cutoff_ms: i64, project_manifests: &[(PathBuf, &PackageManifest)], ) -> bool {🤖 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/package-manager/src/optimistic_repeat_install.rs` around lines 452 - 488, The doc comment describing statting package.json and referencing statManifestFile belongs to manifests_unchanged_since, but it was left before patches_modified_since; move that entire doc comment block so it immediately precedes the manifests_unchanged_since function declaration (remove it from above patches_modified_since), ensuring there are no blank lines or other items between the doc comment and the manifests_unchanged_since function so Rust will attach the docs correctly; keep the patches_modified_since doc comment (if any) with its function.
🤖 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/lib.rs`:
- Around line 167-182: The top-level struct field order is wrong:
`patched_dependencies` must be declared before `ignored_optional_dependencies`
so YAML produced by `serialize_yaml` matches pnpm's root-key order
(pnpmfileChecksum → patchedDependencies → importers); `serialize_yaml` only
canonicalizes map entries not struct field order, so fix this by moving the `pub
patched_dependencies: Option<std::collections::BTreeMap<String, String>>`
declaration to appear before `pub ignored_optional_dependencies` in the lockfile
struct (and update any comments/tests referencing field order accordingly).
---
Outside diff comments:
In `@pacquet/crates/package-manager/src/optimistic_repeat_install.rs`:
- Around line 452-488: The doc comment describing statting package.json and
referencing statManifestFile belongs to manifests_unchanged_since, but it was
left before patches_modified_since; move that entire doc comment block so it
immediately precedes the manifests_unchanged_since function declaration (remove
it from above patches_modified_since), ensuring there are no blank lines or
other items between the doc comment and the manifests_unchanged_since function
so Rust will attach the docs correctly; keep the patches_modified_since doc
comment (if any) with its function.
🪄 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: 8e38e778-3a2e-452c-8148-cd6189ff5a74
📒 Files selected for processing (18)
pacquet/crates/config/src/lib.rspacquet/crates/config/src/tests.rspacquet/crates/lockfile/src/freshness.rspacquet/crates/lockfile/src/freshness/tests.rspacquet/crates/lockfile/src/lib.rspacquet/crates/lockfile/src/save_lockfile/tests.rspacquet/crates/package-manager/src/current_lockfile.rspacquet/crates/package-manager/src/current_lockfile/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/hoisted_dep_graph/tests.rspacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/optimistic_repeat_install.rspacquet/crates/package-manager/src/optimistic_repeat_install/tests.rspacquet/crates/real-hoist/src/tests.rspacquet/crates/resolving-deps-resolver/src/lockfile_reuse/tests.rspnpr/crates/pnpr/src/resolver/resolve.rs
✅ Files skipped from review due to trivial changes (2)
- pacquet/crates/package-manager/src/hoisted_dep_graph/tests.rs
- pacquet/crates/real-hoist/src/tests.rs
🚧 Files skipped from review as they are similar to previous changes (12)
- pacquet/crates/resolving-deps-resolver/src/lockfile_reuse/tests.rs
- pnpr/crates/pnpr/src/resolver/resolve.rs
- pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs
- pacquet/crates/config/src/tests.rs
- pacquet/crates/package-manager/src/install.rs
- pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs
- pacquet/crates/lockfile/src/save_lockfile/tests.rs
- pacquet/crates/package-manager/src/current_lockfile.rs
- pacquet/crates/lockfile/src/freshness.rs
- pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
- pacquet/crates/lockfile/src/freshness/tests.rs
- pacquet/crates/config/src/lib.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). (5)
- GitHub Check: ubuntu-latest / Node.js 24 / Test
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Code Coverage
- GitHub Check: Run benchmark on ubuntu-latest
🧰 Additional context used
📓 Path-based instructions (1)
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: Log emissions are part of matching pnpm — when porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(...), orstreamParser.write(...), mirror the call site, payload, and ordering so@pnpm/cli.default-reporterparses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing the brand into a plainStringor&strin Rust
If upstream TypeScript always validates before construction of a branded string, validate in the Rust wrapper too viaTryFrom<String>and/orFromStrand do not provide an infallible public constructor
If upstream TypeScript never validates a branded string, just brand for type-safety in Rust by exposing an infallibleFrom<String>constructor
If upstream TypeScript occasionally constructs a branded string without validation, exposefrom_str_uncheckedin Rust as an escape hatch alongside the validating constructor
Match upstream serde behavior for branded strings crossing JSON, YAML, or INI boundaries by using#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization
Derive simple conversions for branded strings using#[derive(derive_more::From)]and#[derive(derive_more::Into)]instead of handwritingimplblocks; use manualimplonly when conversion needs custom logic
Model TypeScript string literal unions (like'auto' | 'always' | 'never') as Rustenums instead of newtype wrappers, since the set of valid values is closed
Treat TypeScript string template literal types (like`${string}@${string}`) the same as branded string types in Rust, using a newtype wrapper with validation
Follow the code style guide inCODE_STYLE_GUIDE.md— imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching,pipe-trait, error handling, test layout, and cloning ofArcandRc
Choose owned vs. borrowed parameters to minimize copies; widen to t...
Files:
pacquet/crates/package-manager/src/current_lockfile/tests.rspacquet/crates/package-manager/src/optimistic_repeat_install.rspacquet/crates/package-manager/src/optimistic_repeat_install/tests.rspacquet/crates/lockfile/src/lib.rs
🧠 Learnings (5)
📚 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/current_lockfile/tests.rspacquet/crates/package-manager/src/optimistic_repeat_install.rspacquet/crates/package-manager/src/optimistic_repeat_install/tests.rspacquet/crates/lockfile/src/lib.rs
📚 Learning: 2026-05-22T00:08:44.646Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.
Applied to files:
pacquet/crates/package-manager/src/current_lockfile/tests.rspacquet/crates/package-manager/src/optimistic_repeat_install.rspacquet/crates/package-manager/src/optimistic_repeat_install/tests.rspacquet/crates/lockfile/src/lib.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/current_lockfile/tests.rspacquet/crates/package-manager/src/optimistic_repeat_install.rspacquet/crates/package-manager/src/optimistic_repeat_install/tests.rspacquet/crates/lockfile/src/lib.rs
📚 Learning: 2026-06-06T18:58:37.156Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12243
File: pacquet/crates/package-manager/src/install_package_by_snapshot.rs:319-322
Timestamp: 2026-06-06T18:58:37.156Z
Learning: When reviewing Rust code, do not assume `matches!(expr, Pattern(_))` will move out of `expr` if `Pattern(_)` contains no by-value bindings. `matches!` desugars to a `match` that auto-borrows the scrutinee for discrimination, so even if `expr` is a non-`Copy` value behind a shared reference (e.g., `&T`), the macro should not move-out of the borrowed data purely due to `matches!`. Treat `matches!(&expr, Pattern(_))` as a readability/clarity improvement, not a correctness requirement. Only flag potential move-out-of-borrow risks when the pattern includes by-value bindings that would require moving the matched value.
Applied to files:
pacquet/crates/package-manager/src/current_lockfile/tests.rspacquet/crates/package-manager/src/optimistic_repeat_install.rspacquet/crates/package-manager/src/optimistic_repeat_install/tests.rspacquet/crates/lockfile/src/lib.rs
📚 Learning: 2026-05-20T23:23:40.606Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11788
File: pacquet/crates/lockfile/src/snapshot_dep_ref.rs:52-56
Timestamp: 2026-05-20T23:23:40.606Z
Learning: In the pacquet lockfile crate, the link target is intentionally represented as a raw `Link(String)` (not a `LinkTarget` newtype) for both `SnapshotDepRef::Link` and `ImporterDepVersion::Link`. pnpm upstream preserves `link:` targets verbatim with only a prefix check (e.g., `refToRelative`), so do not suggest introducing a `LinkTarget` newtype in a bug-fix/targeted PR. If type branding is ever desired, it should be done together for both variants in a separate dedicated refactor PR rather than as part of a small fix.
Applied to files:
pacquet/crates/lockfile/src/lib.rs
🔇 Additional comments (7)
pacquet/crates/lockfile/src/lib.rs (1)
140-152: LGTM!pacquet/crates/package-manager/src/current_lockfile/tests.rs (1)
54-68: LGTM!pacquet/crates/package-manager/src/optimistic_repeat_install.rs (3)
18-31: LGTM!
157-165: LGTM!
468-488: LGTM!pacquet/crates/package-manager/src/optimistic_repeat_install/tests.rs (2)
628-674: LGTM!
676-720: LGTM!
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
PR Summary by Qodo
Emit patchedDependencies in pacquet-generated pnpm-lock.yaml
✨ Enhancement🧪 Tests🕐 20-40 MinutesWalkthroughs
User Description
What
Ports the top-level
patchedDependencies:lockfile block to pacquet — item 6 of the pacquet lockfile-parity tracking issue, #12266.Why
pacquet install --lockfile-onlyalready resolved and hashed patches (for the depPath(patch_hash=…)suffix and at build time) but never wrote the top-levelpatchedDependencies:block intopnpm-lock.yaml. So a clean-state lockfile diverged from pnpm's, which records e.g.:How
pacquet-lockfile— addpatched_dependencies: Option<BTreeMap<String, String>>to theLockfilestruct, in itssortLockfileKeysslot (betweenpnpmfileChecksumandimporters).yaml_emitalready sorts this section's keys, so it serializes byte-faithfully with no dumper change.pacquet-config— addConfig::patched_dependency_hashes(), a port of pnpm'scalcPatchHashes(opts.patchedDependencies): resolve each patch path against the workspace dir and hash it, keeping the user's verbatim keys. This is distinct fromresolved_patched_dependencies()(which buckets by package name) so a barefooandfoo@*stay separate lockfile keys rather than collapsing into one group bucket.pacquet-package-manager— compute the hashes once per install and thread them throughGraphToLockfileOptions→dependencies_graph_to_lockfile; the current lockfile (lock.yaml) carries them through.Testing
None), and a byte-parity save test modeling the repo's owngraceful-fs@4.2.11entry.68ebc232…) matches the committedpnpm-lock.yamlexactly.pacquet-lockfile(449) andpacquet-configsuites pass;install::tests(103) pass; clippy / fmt / taplo / typos clean; full-workspacecargo check --all-targetsclean.This is a pacquet-only catch-up to the TS pnpm CLI (which already emits this block), so no TS-side change and no changeset are needed.
Written by an agent (Claude Code, claude-opus-4-8).
AI Description
Diagram
High-Level Assessment
The following are alternative approaches to this PR:
1. Return hashes alongside grouped patch records
2. Compute patchedDependencies hashes at lockfile serialization time
3. Reuse resolved_patched_dependencies output to derive lockfile map
Recommendation: Current approach is the best fit for pnpm parity: compute a verbatim key→hash map once per install (where filesystem context and error handling belong), keep it distinct from the grouped resolver view, and thread it into lockfile generation with empty-map normalization. The main thing to double-check in review is that all code paths that write/filter lockfiles consistently preserve/omit the field and that error surfaces are appropriately mapped (CalcPatchHashes).
File Changes
Enhancement (5)
Tests (7)
Summary by CodeRabbit
New Features
Behavior Changes
Tests