lockfile: resolve a read against the declared package manager, not filename precedence - #781
lockfile: resolve a read against the declared package manager, not filename precedence#781colinhacks wants to merge 2 commits into
Conversation
…lename precedence `parse_lockfile_with_kind_and_options` walked `lockfile_candidates` in raw filename precedence and read whichever file existed first, while the write path asks the declaration-aware `resolve_project_lockfile_kind`. In a project declaring npm, a `bun.lock` sitting beside `package-lock.json` outranks it in that precedence, so an install resolved against bun's graph and serialized it back out as `package-lock.json` — dropping `resolved`, `license` and `engines` from every entry bun's format cannot carry, and installing bun's pins rather than the project's own. Reorder the read candidates so the declaration's family leads, falling back to today's precedence when detection errors, so a contradicted or ambiguous project still reaches the write path's existing error instead of a new one here. Separately, the npm writer's root importer entry mirrored only the manifest's dependency fields. npm also copies `license`, `bin` and `engines` there, so a genuine rewrite dropped all three from a hand-written lockfile regardless of any foreign lockfile. Mirror them, with npm's normalization of an object `license` and a string `bin`, verified byte-for-byte against npm 11.17.
|
Your Claude subscription has hit its usage limit. It resets at 11pm (UTC). Re-trigger Pullfrog after the reset, or add an Add repo secret → · Model settings → · Setup docs → · Ask in Discord →
|
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
npm normalizes every bin path in the root importer entry, stripping the leading `./` and stripping it repeatedly — `././d.js` lands as `d.js`. Mirroring the manifest verbatim wrote `./cli.js` where npm writes `cli.js`, so an alternating npm/nub install churned the lockfile on exactly the entry this branch set out to stop churning. Verified against npm 11.17 across all four spellings (`./a.js`, `b.js`, `./nested/c.js`, `././d.js`): byte-identical bin maps. The test fails with the normalization reverted.
|
Your Claude subscription has hit its usage limit. It resets at 11pm (UTC). Re-trigger Pullfrog after the reset, or add an Add repo secret → · Model settings → · Setup docs → · Ask in Discord →
|
The defect
In a project declaring npm, a
bun.locksitting besidepackage-lock.jsonmadenub installrewritepackage-lock.jsonwithout theresolved,licenseandenginesof exactly the entriesbun.lockalso names. It also installed bun's pins rather than the project's own. Remove thebun.lockand the same fixture round-tripped byte-for-byte.The same shape reproduces with a stray
pnpm-lock.yamloryarn.lock, and withnpm-shrinkwrap.json.Root cause
Reads and writes were resolving different files. The write path asks the declaration-aware
resolve_project_lockfile_kind, which honorspackageManageranddevEngines. The read path walked candidates in raw filename precedence —pnpm > bun > yarn > npm-shrinkwrap > npm— and took the first that existed. Sincepackage-lock.jsonsorts last, the stray file won the read while npm still won the write, so bun's thinner graph was serialized into npm's file.Confirmed with
RUST_LOG=debug: the install loggedpeer-context pass (lockfile=Bun)before,lockfile=Npmafter. Nothing merged the two lockfiles — the npm one was never read.The read candidates are now ordered so the declaration's family leads. Reordering rather than filtering keeps this from inventing a failure mode: when a declaration contradicts the disk, or several tools' lockfiles coexist undeclared, detection errors and the read falls back to the old precedence, leaving the write path to raise the error it already raised.
Second defect, independent of any foreign lockfile
The npm writer's root importer entry mirrored only the manifest's dependency fields. Real npm also copies
license,binandenginesthere, so a genuine rewrite dropped all three even with no stray lockfile present. Both npm normalizations are reproduced: an objectlicensecollapses to itstype, a stringbinexpands to{ <name-without-scope>: <path> }. Scoped to the root importer — a workspace member's entry has no manifest available on the path that matters.The signal-exit hoist: intended, left alone
The same rewrite moved
node_modules/write-file-atomic/node_modules/signal-exittonode_modules/signal-exit. Differential on the same fixture:node_modules/write-file-atomic/node_modules/signal-exitnode_modules/signal-exitnode_modules/signal-exitThe npm writer recomputes the layout from the flat graph on every write and places each package in the shallowest legal slot. Only one version of
signal-exitis demanded in the tree and the root slot is free, so the hoist is what npm itself produces on a fresh resolve. npm differs only in that its tree build is incremental and preserves an existing valid placement.Verification
Npm,package-lock.jsonbyte-identical.nub add is-odd@3.0.1) withbun.lockpresent: zeroresolved/license/engineslost, against 17 fields before.license,binandengines: nub'spackage-lock.jsonbyte-identical to npm 11.17's.--all-targets --all-features,cargo fmt --check, rootcargo test(59 binaries), andcargo test --workspaceinvendor/aube(56 binaries) all pass.