This repository was archived by the owner on May 14, 2026. It is now read-only.
feat(cmd-shim,package-manager): hoisted-bin precedence + post-build top-level bin re-link (#342)#523
Merged
Conversation
…op-level bin re-link (#342) Two related bin-linking behaviors deferred from #333. Behavior 1 — hoisted-bin precedence: - Add `BinOrigin { Direct, Hoisted }` discriminator to `PackageBinSource`. - New top tier in `pick_winner`: Direct wins outright over Hoisted regardless of ownership / lexical order. Mirrors upstream's `preferDirectCmds` partition at https://github.com/pnpm/pnpm/blob/4750fd370c/bins/linker/src/index.ts#L92. - New `link_top_level_bins(modules_dir, direct, hoisted)` helper in `pacquet-package-manager` mixes both candidate lists into one `link_bins_of_packages` call so the new tier resolves conflicts in a single pass — previously the two passes (SymlinkDirectDependencies for direct + hoist pass for publicly-hoisted) wrote shims independently and a hoisted bin could shadow a direct one when its package name was lexically smaller. Behavior 2 — lifecycle-script-created bins: - Add a post-`BuildModules` per-importer top-level bin link pass. Re-reading each direct dep's `package.json` after lifecycle scripts run picks up bins generated by `postinstall` (the `@pnpm.e2e/generated-bins` upstream fixture). Idempotent for unchanged shims via `is_shim_pointing_at`. - Mirrors upstream's `linkBinsOfImporter` pass that runs after `buildModules` at https://github.com/pnpm/pnpm/blob/4750fd370c/installing/deps-installer/src/install/index.ts#L1539. Supporting changes: - `PackageBinSource::new(location, manifest)` constructor + `with_origin` builder so existing call sites don't have to spell out the new field. - Public `direct_dep_names_for_importer` helper extracted from `SymlinkDirectDependencies` so the post-build pass uses the same filter (skipped / first-wins / link_only) as the symlink phase. - `InstallFrozenLockfileError::TopLevelBinLink` for the new failure surface. Tests: - `direct_origin_wins_over_hoisted_regardless_of_lexical` — pins the new tier overrides lexical ordering. - `hoisted_origin_loses_to_existing_direct` — pins both arms of the new tier (Direct incumbent vs Hoisted candidate).
|
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 (5)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (5)📚 Learning: 2026-05-07T23:19:08.272ZApplied to files:
📚 Learning: 2026-05-13T22:52:32.579ZApplied to files:
📚 Learning: 2026-05-13T19:22:48.951ZApplied to files:
📚 Learning: 2026-05-13T20:09:22.171ZApplied to files:
📚 Learning: 2026-05-01T10:01:33.766ZApplied to files:
🔇 Additional comments (34)
📝 WalkthroughWalkthroughThis PR introduces origin-based precedence to bin conflict resolution: ChangesBin Origin Tracking and Conflict Resolution
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 |
Micro-Benchmark ResultsLinux |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #523 +/- ##
==========================================
+ Coverage 89.09% 89.15% +0.06%
==========================================
Files 125 125
Lines 14114 14220 +106
==========================================
+ Hits 12575 12678 +103
- Misses 1539 1542 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.5571314427200003,
"stddev": 0.10575257351901567,
"median": 2.5250070007200005,
"user": 2.6016254799999996,
"system": 3.55396104,
"min": 2.45645085622,
"max": 2.7832007422200005,
"times": [
2.63340076922,
2.4748258672200003,
2.58425646022,
2.45645085622,
2.63900439322,
2.7832007422200005,
2.4807273442200004,
2.4814983832200004,
2.56851561822,
2.4694339932200005
]
},
{
"command": "pacquet@main",
"mean": 2.48912010652,
"stddev": 0.04529722748045105,
"median": 2.50069008722,
"user": 2.59338798,
"system": 3.5077166399999995,
"min": 2.41800490522,
"max": 2.5419946072200004,
"times": [
2.5419946072200004,
2.53278972922,
2.51242031122,
2.4723078542200003,
2.46246218922,
2.4889598632200003,
2.41800490522,
2.41846177122,
2.51812004922,
2.5256797852200004
]
},
{
"command": "pnpm",
"mean": 5.75673943652,
"stddev": 0.09362874310307445,
"median": 5.71631575722,
"user": 8.31341888,
"system": 4.24166814,
"min": 5.656685631219999,
"max": 5.94215824322,
"times": [
5.79468007722,
5.656685631219999,
5.71673722622,
5.7158942882199995,
5.69579782022,
5.87200059322,
5.80189870022,
5.94215824322,
5.66724066822,
5.70430111722
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.73224684468,
"stddev": 0.017930788855753135,
"median": 0.72466200378,
"user": 0.36271014,
"system": 1.62247766,
"min": 0.7095673892800001,
"max": 0.7709113662800001,
"times": [
0.7709113662800001,
0.7221272012800001,
0.7485218242800001,
0.7240391432800001,
0.7095673892800001,
0.7337604682800001,
0.7451693922800001,
0.7227239272800001,
0.72528486428,
0.7203628702800001
]
},
{
"command": "pacquet@main",
"mean": 0.81961317558,
"stddev": 0.0631607831573082,
"median": 0.79127773178,
"user": 0.36684083999999995,
"system": 1.6390974599999997,
"min": 0.7441211822800001,
"max": 0.94730259528,
"times": [
0.8610388392800001,
0.8498539632800001,
0.7702147132800001,
0.7776181402800001,
0.7441211822800001,
0.94730259528,
0.79556891228,
0.7869865512800001,
0.7810671342800001,
0.8823597242800001
]
},
{
"command": "pnpm",
"mean": 2.4168941028799997,
"stddev": 0.15437208679301562,
"median": 2.3745173132799997,
"user": 2.80340564,
"system": 2.1725686600000005,
"min": 2.27515265428,
"max": 2.79536933228,
"times": [
2.46905985828,
2.3723921772799996,
2.79536933228,
2.37664244928,
2.36563762028,
2.2794311212799996,
2.3875328582799997,
2.27515265428,
2.31817756228,
2.52954539528
]
}
]
} |
This was referenced May 14, 2026
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 the two bin-linking behaviors deferred from #333:
Behavior 1 — Hoisted-bin precedence
Direct dependencies' bins must win over publicly-hoisted (transitive) bins with the same name, so a project never gets its own tooling silently shadowed by a transitive's bin. Mirrors upstream's
preferDirectCmdspartition.BinOrigin { Direct, Hoisted }discriminator onPackageBinSource.pick_winnergains a top tier: Direct wins outright over Hoisted regardless of ownership / lexical order. Existing tiers (ownership, then lexical fallback) still apply within each origin bucket.link_top_level_bins(modules_dir, direct_names, hoisted_names)helper inpacquet-package-managermixes both candidate lists into onelink_bins_of_packagescall so the new tier resolves conflicts in a single pass. Previously the two passes (SymlinkDirectDependenciesfor direct + hoist pass for publicly-hoisted) wrote shims independently and a hoisted bin could shadow a direct one when its package name was lexically smaller.Behavior 2 — Lifecycle-script-created bins
Some packages generate their bin files via
postinstall(the@pnpm.e2e/generated-binsfixture upstream uses to test this). The earlier per-importer bin link runs before lifecycle scripts, so generated bins were missed.BuildModulesper-importer top-level bin link pass. Re-reading each direct dep'spackage.jsonafter lifecycle scripts run picks up newly-found bin entries that point at now-existing files.is_shim_pointing_at, so the second pass is essentially free for installs without generated bins or precedence conflicts.linkBinsOfImporterpass that runs afterbuildModules.Supporting changes
PackageBinSource::new(location, manifest)constructor +with_originbuilder so existing call sites don't have to spell out the new field.direct_dep_names_for_importerhelper extracted fromSymlinkDirectDependenciesso the post-build pass uses the same filter (skipped / first-wins / link_only) as the symlink phase.InstallFrozenLockfileError::TopLevelBinLinkfor the new failure surface (distinct fromHoistLinkBinswhich targets the virtual store's private hoist dir).Test plan
just ready(all tests pass; lint, fmt, typos clean)just dylint(perfectionist) cleantaplo format --checkcleanRUSTDOCFLAGS=\"-D warnings\" cargo doc -p pacquet-cmd-shim -p pacquet-package-manager --no-depscleandirect_origin_wins_over_hoisted_regardless_of_lexical— pins the new tier overrides lexical ordering.hoisted_origin_loses_to_existing_direct— pins both arms of the new tier (Direct incumbent vs Hoisted candidate).End-to-end fixture coverage against upstream's
@pnpm.e2e/generated-binsis left to a follow-up — adding the fixture to the registry-mock is its own task. The unit tests pin the precedence and the post-build pass's idempotency contract.Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
New Features
Bug Fixes