fix: contain hoisted dependency aliases (GHSA-fr4h-3cph-29xv)#12343
Conversation
The `nodeLinker: hoisted` install restores its dependency graph straight from the lockfile via `lockfileToHoistedDepGraph`, which joins each dependency alias under a `node_modules` directory and imports the package files there. On a frozen / up-to-date lockfile, resolution is skipped entirely, so the alias validation added for the resolution path never runs. A crafted lockfile alias such as `../../../escape` could therefore escape the install root, and reserved aliases such as `.bin`, `.pnpm`, or `node_modules` could overwrite pnpm-owned layout. Validate every alias at the hoisted-graph directory sink. The shared `safeJoinModulesDir` helper now rejects aliases that are not valid npm package names (path-traversal, absolute, and reserved names) in addition to its containment check, and the hoisted graph routes its `dep.name` sink through it. Pacquet mirrors the boundary: `safe_join_modules_dir` validates the hoister's `dep.0.name` before adding the graph node or recursing, reusing the same dependency-name rule it already applies to direct-dependency aliases. Both stacks surface `ERR_PNPM_INVALID_DEPENDENCY_NAME`. --- Written by an agent (Claude Code, claude-fable-5).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (2)📚 Learning: 2026-05-14T09:04:00.133ZApplied to files:
📚 Learning: 2026-06-05T13:47:26.046ZApplied to files:
🔇 Additional comments (6)
📝 WalkthroughWalkthroughThis PR adds dual-layer protection against malicious dependency aliases: an always-on lockfile scan that rejects invalid alias keys, and safe-join helpers used by hoisted graph builders to validate aliases before creating node_modules paths. Changes touch both pnpm (TypeScript) and pacquet (Rust) and include tests. ChangesDependency Alias Containment
Sequence DiagramsequenceDiagram
participant Client as pnpm install
participant VLR as verifyLockfileResolutions
participant Restorer as lockfileToHoistedDepGraph
participant SafeJoinTS as safeJoinModulesDir
participant SafeJoinRS as safe_join_modules_dir
participant FS as node_modules filesystem
Client->>VLR: provide lockfile
VLR->>VLR: scan importer & snapshot alias keys
VLR-->>Client: reject if invalid aliases found
Client->>Restorer: build hoisted graph
Restorer->>SafeJoinTS: safeJoinModulesDir(modules, alias)
Restorer->>SafeJoinRS: safe_join_modules_dir(modules, alias)
SafeJoinTS->>SafeJoinTS: validate alias name
SafeJoinRS->>SafeJoinRS: validate alias name
SafeJoinTS-->>Restorer: return safe path or error
SafeJoinRS-->>Restorer: return safe path or error
SafeJoinTS->>FS: use returned path
SafeJoinRS->>FS: use returned path
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 |
Add an always-on, policy-independent structural check to verifyLockfileResolutions that rejects any importer or package-snapshot dependency alias that is not a valid npm package name. A dependency alias becomes a `node_modules/<alias>` directory at link time, so an alias with path-traversal segments or a reserved name (`.bin`, `.pnpm`, `node_modules`) could escape the install root or overwrite pnpm-owned layout. This complements the linker-sink guards: the verifier runs before any fetch or filesystem work and covers every node linker at once, while the sink guards still protect the `trustLockfile` path the verifier skips. The check runs before the cache lookup so a record written by a version that predates the rule cannot fast-path around it, and before the `packages` guard so a tampered importer alias is caught even when nothing is installed. `isValidDependencyAlias` is now exported from `@pnpm/installing.deps-resolver` and reused here. Pacquet mirrors the gate in its lockfile-verification crate with a matching `ERR_PNPM_INVALID_DEPENDENCY_NAME` verdict. --- Written by an agent (Claude Code, claude-fable-5).
Micro-Benchmark ResultsLinux |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12343 +/- ##
==========================================
+ Coverage 87.74% 87.87% +0.12%
==========================================
Files 291 292 +1
Lines 36107 36314 +207
==========================================
+ Hits 31683 31911 +228
+ Misses 4424 4403 -21 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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": 4.377330827820001,
"stddev": 0.2781173100356417,
"median": 4.402347111120001,
"user": 2.8573838,
"system": 2.6121643800000003,
"min": 3.9768500341200004,
"max": 4.96535384612,
"times": [
4.08793296312,
4.24170967512,
4.416587214120001,
4.38810700812,
4.53162370612,
3.9768500341200004,
4.96535384612,
4.44705739812,
4.52763864312,
4.19044779012
]
},
{
"command": "pacquet@main",
"mean": 4.13005415072,
"stddev": 0.10041743366041282,
"median": 4.14273492662,
"user": 2.9045291000000004,
"system": 2.64640808,
"min": 3.96608451712,
"max": 4.26880467312,
"times": [
4.23807367512,
4.21336441212,
4.16106350312,
4.12440635012,
3.96608451712,
4.17123762812,
4.26880467312,
4.04279912012,
4.10801301212,
4.00669461612
]
},
{
"command": "pnpr@HEAD",
"mean": 2.5979040776199995,
"stddev": 0.21344351046027205,
"median": 2.53644944912,
"user": 2.1107948000000003,
"system": 2.3223705799999994,
"min": 2.31731459612,
"max": 3.03099437512,
"times": [
3.03099437512,
2.4909043311200003,
2.70864599112,
2.3869260061200004,
2.31731459612,
2.5321111101200002,
2.77479906812,
2.46296807712,
2.73358943312,
2.5407877881200003
]
},
{
"command": "pnpr@main",
"mean": 3.06961543292,
"stddev": 0.586227140249338,
"median": 2.9854653661200006,
"user": 2.1186835000000004,
"system": 2.31509508,
"min": 2.35300539912,
"max": 4.0536466221200005,
"times": [
2.8709139311200005,
2.57406195212,
2.35300539912,
2.62082791112,
3.7803643081200002,
2.56373531412,
3.10001680112,
3.10906030912,
3.67052178112,
4.0536466221200005
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.738645417,
"stddev": 0.09385516840763992,
"median": 0.7187194625000001,
"user": 0.30964334,
"system": 1.0490084400000002,
"min": 0.649156441,
"max": 0.906504492,
"times": [
0.8619985370000001,
0.6504922180000001,
0.906504492,
0.675256657,
0.7847431060000001,
0.649156441,
0.6636390200000001,
0.7744652510000001,
0.7621822680000001,
0.6580161800000001
]
},
{
"command": "pacquet@main",
"mean": 0.8836337962,
"stddev": 0.28453077068087823,
"median": 0.7767645575000002,
"user": 0.30864724,
"system": 1.0547681399999997,
"min": 0.6611154010000001,
"max": 1.627424506,
"times": [
0.6664956650000001,
0.774752069,
0.6611154010000001,
0.8593267700000001,
0.8734742150000001,
0.7785156500000001,
0.7653131140000001,
0.7750134650000001,
1.054907107,
1.627424506
]
},
{
"command": "pnpr@HEAD",
"mean": 0.9592326386000002,
"stddev": 0.18709076440574854,
"median": 0.8779011320000001,
"user": 0.32599673999999995,
"system": 1.0421471400000002,
"min": 0.7579202760000001,
"max": 1.241372394,
"times": [
0.8842513070000001,
0.8715509570000001,
0.8323232970000001,
1.233282338,
0.7579202760000001,
0.8280699950000001,
0.8594328980000001,
0.8847516930000001,
1.241372394,
1.199371231
]
},
{
"command": "pnpr@main",
"mean": 0.8930337122,
"stddev": 0.11380254664543527,
"median": 0.8810808415000001,
"user": 0.31779714,
"system": 1.04850554,
"min": 0.747314934,
"max": 1.0860204,
"times": [
0.7520329590000001,
0.8595523150000001,
0.9407801870000001,
0.8723763600000001,
0.9759753820000001,
0.8897853230000001,
0.747314934,
1.020027372,
1.0860204,
0.7864718900000001
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 3.6613346665599997,
"stddev": 0.05549386786004042,
"median": 3.64159800396,
"user": 2.8264179799999996,
"system": 2.56958342,
"min": 3.60727419346,
"max": 3.77687703346,
"times": [
3.66820642846,
3.70284739846,
3.60727419346,
3.62360790546,
3.71760366146,
3.77687703346,
3.62395344346,
3.65276189046,
3.63043411746,
3.60978059346
]
},
{
"command": "pacquet@main",
"mean": 3.71388029236,
"stddev": 0.1484854379858388,
"median": 3.6836783519600003,
"user": 2.83599028,
"system": 2.59062892,
"min": 3.61324339746,
"max": 4.12136504946,
"times": [
3.68112523946,
3.64031717446,
3.61930050646,
3.71577256146,
3.71390342846,
3.61324339746,
3.68623146446,
3.63605883646,
4.12136504946,
3.71148526546
]
},
{
"command": "pnpr@HEAD",
"mean": 2.54596885546,
"stddev": 0.25808509466500185,
"median": 2.60475199896,
"user": 1.9574768800000002,
"system": 2.2223293199999996,
"min": 2.18816327946,
"max": 2.89546559146,
"times": [
2.18816327946,
2.7013850274599998,
2.75148123746,
2.44648806846,
2.20332433546,
2.53266777146,
2.89546559146,
2.67683622646,
2.2693453464599997,
2.79453167046
]
},
{
"command": "pnpr@main",
"mean": 2.5599563018600002,
"stddev": 0.39345364100633445,
"median": 2.4887551654599998,
"user": 1.9677421800000001,
"system": 2.20640752,
"min": 2.1947882014599998,
"max": 3.52374969046,
"times": [
2.1947882014599998,
2.58602334646,
2.26702731846,
2.36934149846,
2.49970578246,
2.2026633964599998,
2.47780454846,
2.79340751046,
3.52374969046,
2.68505172546
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.2425540150600003,
"stddev": 0.23702670800834613,
"median": 1.1555552106600002,
"user": 0.8571356400000001,
"system": 1.3541041000000003,
"min": 1.0303886126600001,
"max": 1.7534784716600003,
"times": [
1.1314446796600002,
1.0763950666600002,
1.11017764566,
1.23127182666,
1.0436788406600002,
1.0303886126600001,
1.17966574166,
1.7534784716600003,
1.32324464366,
1.5457946216600003
]
},
{
"command": "pacquet@main",
"mean": 1.1367826746600003,
"stddev": 0.1585618961948948,
"median": 1.04946400066,
"user": 0.86279244,
"system": 1.3686142999999997,
"min": 1.03410438466,
"max": 1.4707856166600002,
"times": [
1.03410438466,
1.03535358366,
1.04183077266,
1.1952346636600002,
1.3686397356600002,
1.4707856166600002,
1.0837491856600001,
1.04124843666,
1.03978313866,
1.05709722866
]
},
{
"command": "pnpr@HEAD",
"mean": 0.72309299066,
"stddev": 0.10847209876013326,
"median": 0.67666492966,
"user": 0.26655014,
"system": 1.0094452999999999,
"min": 0.66296142166,
"max": 0.9751309436600001,
"times": [
0.67745057466,
0.66501861466,
0.66296142166,
0.67212353466,
0.67653185066,
0.9751309436600001,
0.8713696186600001,
0.68434941066,
0.66919592866,
0.6767980086600001
]
},
{
"command": "pnpr@main",
"mean": 0.72244252846,
"stddev": 0.06890051842523026,
"median": 0.69678475016,
"user": 0.27460754,
"system": 1.0186591999999999,
"min": 0.66971770266,
"max": 0.8707462556600001,
"times": [
0.69041834566,
0.71153370666,
0.69847137366,
0.69874163366,
0.69509812666,
0.66971770266,
0.8707462556600001,
0.67647565666,
0.82898202066,
0.68424046266
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.41364635162,
"stddev": 0.054478632825727645,
"median": 2.3996336446199997,
"user": 1.2426568199999999,
"system": 1.5983964,
"min": 2.3590983621199997,
"max": 2.55872340912,
"times": [
2.39782143512,
2.3867825471199997,
2.42481558212,
2.42080355912,
2.41236976112,
2.38635178512,
2.40144585412,
2.3590983621199997,
2.38825122112,
2.55872340912
]
},
{
"command": "pacquet@main",
"mean": 2.52436815942,
"stddev": 0.1921451409920944,
"median": 2.4817463536199997,
"user": 1.23076162,
"system": 1.5590362999999996,
"min": 2.3592476851199997,
"max": 2.97093455712,
"times": [
2.5175690671199997,
2.36518371212,
2.59457622012,
2.4459236401199997,
2.3592476851199997,
2.36621061312,
2.37572693612,
2.6565595591199997,
2.59174960412,
2.97093455712
]
},
{
"command": "pnpr@HEAD",
"mean": 0.77539165012,
"stddev": 0.12514807524208826,
"median": 0.71450771412,
"user": 0.26673751999999995,
"system": 1.0166507,
"min": 0.65762981412,
"max": 1.01419119812,
"times": [
0.87050799412,
0.65762981412,
0.67819499312,
0.66532038012,
0.87287432712,
1.01419119812,
0.74674013212,
0.68227529612,
0.68223907312,
0.8839432931200001
]
},
{
"command": "pnpr@main",
"mean": 0.6888916102199999,
"stddev": 0.005082505397228882,
"median": 0.68913811412,
"user": 0.26041152,
"system": 1.0420797999999998,
"min": 0.67922832812,
"max": 0.69636013912,
"times": [
0.68827784112,
0.68928285812,
0.68290086712,
0.67922832812,
0.6889933701200001,
0.69636013912,
0.69136193312,
0.69507611412,
0.68983274612,
0.6876019051200001
]
}
]
} |
|
Bencher Report| Branch | pr/12343 |
| Testbed | pacquet |
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 | 3,661.33 ms(-55.01%)Baseline: 8,137.86 ms | 9,765.44 ms (37.49%) |
| isolated-linker.fresh-install.cold-cache.hot-store | 📈 view plot 🚷 view threshold | 2,413.65 ms(-46.55%)Baseline: 4,515.97 ms | 5,419.16 ms (44.54%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,242.55 ms(-7.76%)Baseline: 1,347.15 ms | 1,616.58 ms (76.86%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 4,377.33 ms(-51.37%)Baseline: 9,000.90 ms | 10,801.08 ms (40.53%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 738.65 ms(+11.02%)Baseline: 665.32 ms | 798.39 ms (92.52%) |
|
| Branch | pr/12343 |
| Testbed | pnpr |
⚠️ WARNING: No Threshold found!Without a Threshold, no Alerts will ever be generated.
Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the--ci-only-thresholdsflag.
Click to view all benchmark results
| Benchmark | Latency | milliseconds (ms) |
|---|---|---|
| isolated-linker.fresh-install.cold-cache.cold-store | 📈 view plot | 2,545.97 ms |
| isolated-linker.fresh-install.cold-cache.hot-store | 📈 view plot | 775.39 ms |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot | 723.09 ms |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot | 2,597.90 ms |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot | 959.23 ms |
`is_valid_dependency_alias` is in scope via `use`, so the bare intra-doc link resolves on its own. The explicit path target tripped `rustdoc::redundant-explicit-links` under the CI Doc job's `cargo doc --document-private-items` (the local pre-push hook runs `cargo doc` without that flag, so it didn't surface). --- Written by an agent (Claude Code, claude-fable-5).
Code Review by Qodo
1.
|
PR Summary by QodoContain hoisted lockfile dependency aliases (GHSA-fr4h-3cph-29xv) WalkthroughsDescription• Reject path-traversal and reserved lockfile dependency aliases before any install work begins. • Guard hoisted linker alias→path joins with npm package-name validation to prevent escaping node_modules. • Add regression/unit tests across pnpm and pacquet for invalid/valid alias scenarios. Diagramgraph TD
L["pnpm-lock.yaml"] --> V["Lockfile verify gate"] --> H["Hoisted dep graph"] --> S["safeJoinModulesDir"] --> NM["node_modules/<alias>"]
L --> PV["Pacquet verify gate"] --> PH["Pacquet hoisted graph"] --> PS["safe_join_modules_dir"] --> NM
High-Level AssessmentThe following are alternative approaches to this PR: 1. Verifier-only (lockfile gate only)
2. Sink-only (safe join at every alias→path join)
3. Canonicalize/sanitize aliases (rewrite to safe names)
Recommendation: Keep the layered approach in this PR: an always-on lockfile structural gate for early, linker-wide rejection, plus sink-level guards for defense-in-depth when verification is skipped and to harden the actual alias→path boundary. This balances early failure (no fetch/FS) with robust containment at the filesystem sink. File ChangesEnhancement (3)
Bug fix (6)
Refactor (1)
Tests (6)
Documentation (1)
Other (3)
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
installing/deps-installer/test/install/verifyLockfileResolutions.ts (1)
484-536: ⚡ Quick winKeep the invalid-alias fixture consistent across
installing/deps-installer/test/install/verifyLockfileResolutions.ts,installing/deps-restorer/test/lockfileToHoistedDepGraph.test.ts,pacquet/crates/package-manager/src/safe_join_modules_dir/tests.rs,pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions/tests.rs, andpacquet/crates/package-manager/src/hoisted_dep_graph/tests.rs. These suites are all pinning the same security contract, but the newly added tables diverge and currently omit other invalid package names like""and"."that are already covered infs/symlink-dependency/test/safeJoinModulesDir.test.ts. Reusing one shared corpus, or at least adding those cases everywhere, would make the cross-layer regression coverage much harder to drift.🤖 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 `@installing/deps-installer/test/install/verifyLockfileResolutions.ts` around lines 484 - 536, The invalid-alias fixture needs to include the same set of bad names across suites; update the test data used by the rejecting tests so it contains the missing cases (e.g. "" and ".") in the test.each array in the rejects-an-importer-dependency-alias test and in the nested-package snapshot used in the rejects-an-invalid-alias-nested-in-a-package-snapshot test (the lockfile/specifiers/dependencies entries created in verifyLockfileResolutions tests), and mirror those additions in the other suites named in the comment (installing/deps-restorer..., pacquet/.../tests.rs, etc.) so all tests that assert ERR_PNPM_INVALID_DEPENDENCY_NAME use the identical corpus of invalid names; locate the arrays and lockfile fixtures around the test names "rejects an importer dependency alias %p" and "rejects an invalid alias nested in a package snapshot" and add the missing entries.
🤖 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 `@installing/deps-installer/test/install/verifyLockfileResolutions.ts`:
- Around line 484-536: The invalid-alias fixture needs to include the same set
of bad names across suites; update the test data used by the rejecting tests so
it contains the missing cases (e.g. "" and ".") in the test.each array in the
rejects-an-importer-dependency-alias test and in the nested-package snapshot
used in the rejects-an-invalid-alias-nested-in-a-package-snapshot test (the
lockfile/specifiers/dependencies entries created in verifyLockfileResolutions
tests), and mirror those additions in the other suites named in the comment
(installing/deps-restorer..., pacquet/.../tests.rs, etc.) so all tests that
assert ERR_PNPM_INVALID_DEPENDENCY_NAME use the identical corpus of invalid
names; locate the arrays and lockfile fixtures around the test names "rejects an
importer dependency alias %p" and "rejects an invalid alias nested in a package
snapshot" and add the missing entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 07346239-2313-4b6e-b271-e6be33eba06a
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
.changeset/contain-hoisted-dependency-aliases.mdfs/symlink-dependency/package.jsonfs/symlink-dependency/src/index.tsfs/symlink-dependency/src/safeJoinModulesDir.tsfs/symlink-dependency/test/safeJoinModulesDir.test.tsinstalling/deps-installer/src/install/verifyLockfileResolutions.tsinstalling/deps-installer/test/install/verifyLockfileResolutions.tsinstalling/deps-resolver/src/index.tsinstalling/deps-restorer/src/lockfileToHoistedDepGraph.tsinstalling/deps-restorer/test/lockfileToHoistedDepGraph.test.tspacquet/crates/lockfile-verification/Cargo.tomlpacquet/crates/lockfile-verification/src/errors.rspacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rspacquet/crates/lockfile-verification/src/verify_lockfile_resolutions/tests.rspacquet/crates/package-manager/src/hoisted_dep_graph.rspacquet/crates/package-manager/src/hoisted_dep_graph/tests.rspacquet/crates/package-manager/src/lib.rspacquet/crates/package-manager/src/safe_join_modules_dir.rspacquet/crates/package-manager/src/safe_join_modules_dir/tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ubuntu-latest / Node.js 24 / Test
- GitHub Check: Run benchmark on ubuntu-latest
🧰 Additional context used
📓 Path-based instructions (4)
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/lib.rspacquet/crates/package-manager/src/safe_join_modules_dir/tests.rspacquet/crates/package-manager/src/safe_join_modules_dir.rspacquet/crates/package-manager/src/hoisted_dep_graph.rspacquet/crates/package-manager/src/hoisted_dep_graph/tests.rspacquet/crates/lockfile-verification/src/verify_lockfile_resolutions/tests.rspacquet/crates/lockfile-verification/src/errors.rspacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use Standard Style with trailing commas, prefer functions over classes, declare functions after they are used (relying on hoisting), limit functions to no more than two or three arguments, and use a single options object for functions needing more parameters
Follow import order: standard libraries first, then external dependencies (sorted alphabetically), then relative imports
Do not write comments that restate what the code already says; rename variables, split helpers, or move checks to more obvious places instead
Do not repeat documentation at call sites that already lives on the callee; update the JSDoc once and let every call site benefit
Use JSDoc for the function's contract (preconditions, postconditions, edge cases, why the function exists), not for re-narrating the function body
Do not record past implementation shape, refactor history, or removed code in comments; use git log and git blame for that information instead
Write comments only when the reason for code is non-obvious, a hidden invariant exists, a workaround for a known bug is needed, or an exception to surrounding pattern is deliberate
Files:
fs/symlink-dependency/src/index.tsinstalling/deps-resolver/src/index.tsinstalling/deps-restorer/src/lockfileToHoistedDepGraph.tsinstalling/deps-installer/test/install/verifyLockfileResolutions.tsfs/symlink-dependency/test/safeJoinModulesDir.test.tsinstalling/deps-restorer/test/lockfileToHoistedDepGraph.test.tsinstalling/deps-installer/src/install/verifyLockfileResolutions.tsfs/symlink-dependency/src/safeJoinModulesDir.ts
pacquet/**/Cargo.toml
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/Cargo.toml: Check whether the workspace already depends on something suitable in[workspace.dependencies]in the rootCargo.tomlbefore adding a new dependency
Keep dependencies at the right level — add a new dependency to the specific crate that needs it, not to the workspace root or shared crate unless multiple crates depend on it
Files:
pacquet/crates/lockfile-verification/Cargo.toml
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
instanceof Errorfor checking if a caught error is an Error object in Jest tests; useutil.types.isNativeError()instead to work across realms
Files:
fs/symlink-dependency/test/safeJoinModulesDir.test.tsinstalling/deps-restorer/test/lockfileToHoistedDepGraph.test.ts
🧠 Learnings (9)
📚 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/lib.rspacquet/crates/package-manager/src/safe_join_modules_dir/tests.rspacquet/crates/package-manager/src/safe_join_modules_dir.rspacquet/crates/package-manager/src/hoisted_dep_graph.rspacquet/crates/package-manager/src/hoisted_dep_graph/tests.rspacquet/crates/lockfile-verification/src/verify_lockfile_resolutions/tests.rspacquet/crates/lockfile-verification/src/errors.rspacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.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/lib.rspacquet/crates/package-manager/src/safe_join_modules_dir/tests.rspacquet/crates/package-manager/src/safe_join_modules_dir.rspacquet/crates/package-manager/src/hoisted_dep_graph.rspacquet/crates/package-manager/src/hoisted_dep_graph/tests.rspacquet/crates/lockfile-verification/src/verify_lockfile_resolutions/tests.rspacquet/crates/lockfile-verification/src/errors.rspacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.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/lib.rspacquet/crates/package-manager/src/safe_join_modules_dir/tests.rspacquet/crates/package-manager/src/safe_join_modules_dir.rspacquet/crates/package-manager/src/hoisted_dep_graph.rspacquet/crates/package-manager/src/hoisted_dep_graph/tests.rspacquet/crates/lockfile-verification/src/verify_lockfile_resolutions/tests.rspacquet/crates/lockfile-verification/src/errors.rspacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.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/lib.rspacquet/crates/package-manager/src/safe_join_modules_dir/tests.rspacquet/crates/package-manager/src/safe_join_modules_dir.rspacquet/crates/package-manager/src/hoisted_dep_graph.rspacquet/crates/package-manager/src/hoisted_dep_graph/tests.rspacquet/crates/lockfile-verification/src/verify_lockfile_resolutions/tests.rspacquet/crates/lockfile-verification/src/errors.rspacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs
📚 Learning: 2026-05-14T09:04:00.133Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11622
File: resolving/npm-resolver/test/publishedBy.test.ts:350-354
Timestamp: 2026-05-14T09:04:00.133Z
Learning: In the pnpm/pnpm repository, ESLint is the authoritative style linter. Do not raise review findings for missing trailing commas in multiline function calls (e.g., `fs.writeFileSync(...)`) when this repo’s ESLint configuration does not report them and lint passes. Prefer deferring to the ESLint results for this specific trailing-comma rule rather than enforcing it manually in code review.
Applied to files:
fs/symlink-dependency/src/index.tsinstalling/deps-resolver/src/index.tsinstalling/deps-restorer/src/lockfileToHoistedDepGraph.tsinstalling/deps-installer/test/install/verifyLockfileResolutions.tsfs/symlink-dependency/test/safeJoinModulesDir.test.tsinstalling/deps-restorer/test/lockfileToHoistedDepGraph.test.tsinstalling/deps-installer/src/install/verifyLockfileResolutions.tsfs/symlink-dependency/src/safeJoinModulesDir.ts
📚 Learning: 2026-06-05T13:47:26.046Z
Learnt from: vsumner
Repo: pnpm/pnpm PR: 12190
File: installing/deps-installer/src/install/index.ts:2337-2343
Timestamp: 2026-06-05T13:47:26.046Z
Learning: In the pnpm/pnpm codebase, `PnpmError` automatically prefixes `err.code` with `ERR_PNPM_` when you pass a code that does not already start with `ERR_PNPM_` (it normalizes `this.code` via `code.startsWith('ERR_PNPM_') ? code : `ERR_PNPM_${code}``). Therefore, during code review you should NOT flag `new PnpmError(...)` call sites for passing a bare error code (e.g., `new PnpmError('FROZEN_STORE_INCOMPATIBLE_WITH_PNPR', ...)`); the resulting `err.code` will still be `ERR_PNPM_FROZEN_STORE_INCOMPATIBLE_WITH_PNPR`.
Applied to files:
fs/symlink-dependency/src/index.tsinstalling/deps-resolver/src/index.tsinstalling/deps-restorer/src/lockfileToHoistedDepGraph.tsinstalling/deps-installer/test/install/verifyLockfileResolutions.tsfs/symlink-dependency/test/safeJoinModulesDir.test.tsinstalling/deps-restorer/test/lockfileToHoistedDepGraph.test.tsinstalling/deps-installer/src/install/verifyLockfileResolutions.tsfs/symlink-dependency/src/safeJoinModulesDir.ts
📚 Learning: 2026-05-05T23:03:04.286Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11479
File: __utils__/scripts/package.json:6-9
Timestamp: 2026-05-05T23:03:04.286Z
Learning: The pattern cross-env NODE_OPTIONS="$NODE_OPTIONS ..." in package.json scripts is an established convention in the pnpm/pnpm repository and is used across many packages (e.g., fs/hard-link-dir, worker, __utils__/scripts). Do not flag this as a cross-platform issue in individual files; if a change is needed, apply it as a repo-wide change in a separate PR. Scope this guidance to all package.json files in the repo; use the minimatch pattern '**/package.json' to identify relevant files and review changes at the repository level rather than per-file.
Applied to files:
fs/symlink-dependency/package.json
📚 Learning: 2026-05-26T21:01:06.666Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11966
File: .changeset/require-tarball-integrity.md:6-6
Timestamp: 2026-05-26T21:01:06.666Z
Learning: In pnpm lockfile-related release notes/docs (especially changeset markdown), preserve URL hostnames exactly as they appear in pnpm-lock.yaml tarball resolution entries—keep hosts like `codeload.github.com`, `bitbucket.org`, and `gitlab.com` in lowercase. Do not “correct” them to title-case/preserve brand capitalization (e.g., LanguageTool rules like `GITHUB` capitalization) because these are literal URL fragments, not platform brand names.
Applied to files:
.changeset/contain-hoisted-dependency-aliases.md
📚 Learning: 2026-06-05T13:47:05.929Z
Learnt from: vsumner
Repo: pnpm/pnpm PR: 12190
File: installing/deps-installer/test/install/frozenStore.ts:2-17
Timestamp: 2026-06-05T13:47:05.929Z
Learning: In the pnpm/pnpm repository, the shared Jest preset keeps `injectGlobals` at its default (`true`), so `test` and `expect` are available as Jest globals. Therefore, reviewers should not flag (or treat as TypeScript/compilation errors) missing `import { test, expect } from 'jest/globals'` when a test file uses `test`/`expect` without importing them. Importing from `jest/globals` may still be used for consistency with sibling files, but it is not required for execution in this repo unless a Jest preset is explicitly configured with `injectGlobals: false`.
Applied to files:
installing/deps-installer/test/install/verifyLockfileResolutions.tsfs/symlink-dependency/test/safeJoinModulesDir.test.tsinstalling/deps-restorer/test/lockfileToHoistedDepGraph.test.ts
🔇 Additional comments (11)
.changeset/contain-hoisted-dependency-aliases.md (1)
1-15: LGTM!fs/symlink-dependency/package.json (1)
40-41: LGTM!Also applies to: 50-51
fs/symlink-dependency/src/safeJoinModulesDir.ts (1)
1-37: LGTM!fs/symlink-dependency/src/index.ts (1)
1-27: LGTM!installing/deps-resolver/src/index.ts (1)
73-73: LGTM!installing/deps-installer/src/install/verifyLockfileResolutions.ts (2)
101-120: LGTM!
232-279: LGTM!installing/deps-restorer/src/lockfileToHoistedDepGraph.ts (1)
11-11: LGTM!Also applies to: 222-222
pacquet/crates/package-manager/src/safe_join_modules_dir.rs (1)
18-44: LGTM!pacquet/crates/package-manager/src/hoisted_dep_graph.rs (1)
329-336: LGTM!Also applies to: 702-702
pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs (1)
26-26: Confirm verifier ↔ sink alias predicate equivalence.
pacquet_resolving_deps_resolver::is_valid_dependency_alias(alias)delegates directly topacquet_resolving_parse_wanted_dependency::is_valid_old_npm_package_name(alias), so both the early lockfile gate and the hoistedsafe_join_modules_dirpath sink enforce the exact same alias validity rules.
… candidate pass The dependency-alias check ran as its own full traversal of the lockfile in addition to collectCandidates' existing pass over every package snapshot. Fold it into that pass instead: collectCandidates now also validates each importer and snapshot dependency alias and returns the invalid ones alongside the resolution-shape violations, so the lockfile is walked once per verification rather than twice. Because collectCandidates runs after the verification-cache lookup, the alias check is now covered by the cache the same way the resolution-shape check is: a new dependencyAliasCheck cache identity makes a record written before this rule existed fail canTrustPastCheck, forcing a re-verification. The shared helper is renamed withOfflineCheckCacheIdentities and appends both offline-structural-check identities. No behavior change for valid lockfiles; the same ERR_PNPM_INVALID_DEPENDENCY_NAME is thrown for invalid aliases. Mirrored in pacquet's lockfile-verification crate. --- Written by an agent (Claude Code, claude-fable-5).
|
Code review by qodo was updated up to the latest commit f2994fd |
There was a problem hiding this comment.
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 `@installing/deps-installer/src/install/verifyLockfileResolutions.ts`:
- Around line 227-228: There are two consecutive JSDoc openers (`/**`) at the
start of the documentation block in verifyLockfileResolutions.ts which produces
a malformed JSDoc; remove the stray leading `/**` so only a single `/**` begins
the doc comment and ensure the block ends with a matching `*/` to restore a
valid JSDoc for the surrounding function/statement.
🪄 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: 319a0ead-aacc-44bf-aac6-eacfb44e9944
📒 Files selected for processing (4)
installing/deps-installer/src/install/recordLockfileVerified.tsinstalling/deps-installer/src/install/verifyLockfileResolutions.tspacquet/crates/lockfile-verification/src/record_lockfile_verified.rspacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
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/lockfile-verification/src/record_lockfile_verified.rspacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use Standard Style with trailing commas, prefer functions over classes, declare functions after they are used (relying on hoisting), limit functions to no more than two or three arguments, and use a single options object for functions needing more parameters
Follow import order: standard libraries first, then external dependencies (sorted alphabetically), then relative imports
Do not write comments that restate what the code already says; rename variables, split helpers, or move checks to more obvious places instead
Do not repeat documentation at call sites that already lives on the callee; update the JSDoc once and let every call site benefit
Use JSDoc for the function's contract (preconditions, postconditions, edge cases, why the function exists), not for re-narrating the function body
Do not record past implementation shape, refactor history, or removed code in comments; use git log and git blame for that information instead
Write comments only when the reason for code is non-obvious, a hidden invariant exists, a workaround for a known bug is needed, or an exception to surrounding pattern is deliberate
Files:
installing/deps-installer/src/install/recordLockfileVerified.tsinstalling/deps-installer/src/install/verifyLockfileResolutions.ts
🧠 Learnings (6)
📚 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/lockfile-verification/src/record_lockfile_verified.rspacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.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/lockfile-verification/src/record_lockfile_verified.rspacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.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/lockfile-verification/src/record_lockfile_verified.rspacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.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/lockfile-verification/src/record_lockfile_verified.rspacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs
📚 Learning: 2026-05-14T09:04:00.133Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11622
File: resolving/npm-resolver/test/publishedBy.test.ts:350-354
Timestamp: 2026-05-14T09:04:00.133Z
Learning: In the pnpm/pnpm repository, ESLint is the authoritative style linter. Do not raise review findings for missing trailing commas in multiline function calls (e.g., `fs.writeFileSync(...)`) when this repo’s ESLint configuration does not report them and lint passes. Prefer deferring to the ESLint results for this specific trailing-comma rule rather than enforcing it manually in code review.
Applied to files:
installing/deps-installer/src/install/recordLockfileVerified.tsinstalling/deps-installer/src/install/verifyLockfileResolutions.ts
📚 Learning: 2026-06-05T13:47:26.046Z
Learnt from: vsumner
Repo: pnpm/pnpm PR: 12190
File: installing/deps-installer/src/install/index.ts:2337-2343
Timestamp: 2026-06-05T13:47:26.046Z
Learning: In the pnpm/pnpm codebase, `PnpmError` automatically prefixes `err.code` with `ERR_PNPM_` when you pass a code that does not already start with `ERR_PNPM_` (it normalizes `this.code` via `code.startsWith('ERR_PNPM_') ? code : `ERR_PNPM_${code}``). Therefore, during code review you should NOT flag `new PnpmError(...)` call sites for passing a bare error code (e.g., `new PnpmError('FROZEN_STORE_INCOMPATIBLE_WITH_PNPR', ...)`); the resulting `err.code` will still be `ERR_PNPM_FROZEN_STORE_INCOMPATIBLE_WITH_PNPR`.
Applied to files:
installing/deps-installer/src/install/recordLockfileVerified.tsinstalling/deps-installer/src/install/verifyLockfileResolutions.ts
🔇 Additional comments (12)
installing/deps-installer/src/install/verifyLockfileResolutions.ts (5)
47-65: LGTM!
171-178: LGTM!
240-245: LGTM!
247-263: LGTM!
386-426: LGTM!installing/deps-installer/src/install/recordLockfileVerified.ts (1)
5-6: LGTM!Also applies to: 34-34
pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs (5)
99-99: LGTM!Also applies to: 144-147
228-249: LGTM!Also applies to: 261-269, 271-287
367-398: LGTM!
219-224: LGTM!Also applies to: 438-438
326-346: Ensure Rustis_valid_old_npm_package_namematches pnpm’svalidForOldPackagesexactly
- pnpm’s TypeScript
isValidDependencyAlias(alias)returnsvalidateNpmPackageName(alias).validForOldPackages, andparseWantedDependencyalso gates alias handling onvalidForOldPackages.push_invalid_aliasesinpacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rsrelies onis_valid_old_npm_package_namefor the “invalid alias” decision; this must implement the samevalidForOldPackagesrules (including rejection of reserved names like.bin/.pnpm/node_modulesand path-traversal/absolute cases) so there’s no security bypass between TS and Rust.pacquet/crates/lockfile-verification/src/record_lockfile_verified.rs (1)
25-25: LGTM!Also applies to: 52-52
…d comments Move `pushInvalidAliases` below `collectCandidates`, following the repo's declare-after-use convention. Collapse the repeated "an alias becomes a node_modules directory, so a traversal/reserved name escapes or overwrites layout" explanation that was copied across the verifier, the hoisted-graph error, and the pacquet mirror down to a single reference each — the full rationale lives once in the validating sink (`safeJoinModulesDir` / `safe_join_modules_dir`) and the user-facing error hints. --- Written by an agent (Claude Code, claude-fable-5).
|
Code review by qodo was updated up to the latest commit af36ba0 |
…12504) Backports three merged security fixes from main to release/10: - Contain hoisted dependency aliases so a crafted lockfile alias cannot escape node_modules or overwrite pnpm-owned layout. The hoisted graph builder now validates each alias at the safeJoinModulesDir sink. (GHSA-fr4h-3cph-29xv, #12343) - Contain pnpm patch-remove deletions to the configured patches directory. (#12341) - Reject path-traversal config dependency names and versions from pnpm-workspace.yaml before they are used to build filesystem paths. (GHSA-qrv3-253h-g69c, #12470) Written by an agent (Claude Code, claude-opus-4-8).
Summary
The
nodeLinker: hoistedinstall restores its dependency graph straight from the lockfile vialockfileToHoistedDepGraph, which joins each dependency alias under anode_modulesdirectory and imports the package files there. On a frozen / up-to-date lockfile, resolution is skipped entirely, so the alias validation added for the resolution path in #11954 never runs.A crafted lockfile alias such as
../../../escapecould therefore be joined directly under a hoistednode_modulesdirectory and escape the install root, while reserved aliases such as.bin,.pnpm, ornode_modulescould overwrite pnpm-owned layout. Note the existing containment check alone is insufficient for the reserved-name case:.binresolves insidenode_modules, so only package-name validation rejects it.Fix
The fix adds two complementary layers, in both stacks.
1. Linker sink guard
fs/symlink-dependency/src/safeJoinModulesDir.tsnow rejects aliases that are not valid npm package names (path-traversal, absolute, and reserved names) viavalidate-npm-package-name, in addition to its existing containment check. The hoisted-graphdep.namesink inlockfileToHoistedDepGraph.tsnow goes through it instead of a rawpath.join.safe_join_modules_dirhelper reusing the resolver'sis_valid_dependency_alias;hoisted_dep_graph.rsvalidates the hoister'sdep.0.namebefore adding the graph node or recursing.2. Lockfile verification gate (defense in depth)
verifyLockfileResolutionsgains an always-on, policy-independent structural check that rejects any importer or package-snapshot dependency alias that is not a valid package name. It runs before the cache lookup (so a record written by a version predating the rule can't fast-path around it) and before thepackagesguard (so a tampered importer alias is caught even when nothing is installed), failing the install early — before any fetch or filesystem work — for every node linker at once.The two layers are complementary:
trustLockfiledisables the verifier but the sink guards still run; the sink guards are per-linker but the verifier covers a future linker that might add a new alias→path sink.isValidDependencyAliasis now exported from@pnpm/installing.deps-resolver; pacquet mirrors the gate in itslockfile-verificationcrate. Other name-keyed maps (overrides,patchedDependencies, peer deps) are deliberately excluded — they don't become directories.Both stacks surface
ERR_PNPM_INVALID_DEPENDENCY_NAME/INVALID_DEPENDENCY_NAME.Tests
@pnpm/fs.symlink-dependency: helper tests extended with reserved-name aliases and valid positive controls (25 passed).@pnpm/installing.deps-restorer: exploit-regression test asserting the hoisted graph rejects traversal/reserved aliases before any fetch or filesystem work (6 passed, verified failing without the fix). Existingnode-linker=hoistedinstall controls still pass.@pnpm/installing.deps-installer: verifier tests for invalid importer aliases, invalid snapshot-nested aliases, and valid positive controls (35 passed).safe_join_modules_dirunit tests +walker_rejects_invalid_hoisted_alias(package-manager, 449 lib tests passed); verifier-gate alias tests (lockfile-verification, 43 passed).cargo fmt,cargo doc -D warnings,cargo clippy --deny warnings, and dylint all clean.Written by an agent (Claude Code, claude-fable-5).
Summary by CodeRabbit
Bug Fixes
Tests