Repository navigation
pnpm reader follow-ups from #2009's review #2044
Description
Follow-ups from the /code-review high pass on #2009 (pnpm readers through one walk, lock-model PR 2a). The owner settled the main question there: an env-document ref is withheld when the project document of the same lock resolves the same release elsewhere. These lower-impact findings were left out of #2009 on purpose. Paths are as of the lock-model/2a-pnpm-readers branch.
- Custom patch-server origins under
lockfile=false.withhold_hosted_pins(crates/socket-patch-core/src/vendor/lock_inventory/npm_family.rs) callshosted_patch_uuid(url, &[]). Underlockfile=false, a pin from a custom--patch-server-urlorigin keeps its integrity, while discovery withholds it. The inventory has no per-run origin list yet (lock-model PR 12). - Dedup rank after the integrity drop. Dropping a hosted entry's integrity under
lockfile=falselowers its rank indedup_prefer_integrity(crates/socket-patch-core/src/vendor/lock_inventory/mod.rs). A member or Rush registry duplicate that has integrity can then replace the hosted instance in the collapsed inventory. - Config-only entries count as lock-resolvable for
vendor.PnpmLock::entries()(crates/socket-patch-core/src/formats/pnpm/mod.rs) now lists env-document (config-dependency) packages. The vendored planners (vendor/pnpm_lock.rs,vendor/pnpm_lock_legacy.rs) wire only the project document, so vendoring a package that exists only as a config dependency fails late instead of withpackage_not_installed. -
UnwiredCopyscope vs theUnattestedkey.UnwiredCopyis scoped per(file, TreePart)(crates/socket-patch-core/src/vex/discover/mod.rs), but theUnattestedmark it produces is keyed by(purl, uuid). An env-tree bundled-copy mark therefore also withholds the project tree's ref of the same patch. This direction fails closed, so it is a missed attestation, never a false one. The reverse direction (project copies withholding env refs) is now intended (Read and plan pnpm locks through one core (lock-model 2) #2009). -
is_env_documentvscheck_env_documentdisagree.grammar::is_env_document(crates/socket-patch-core/src/formats/pnpm/grammar.rs) needs at least one env field and is CRLF-aware.lines::check_env_document(crates/socket-patch-core/src/formats/pnpm/lines.rs) accepts zero fields. They can disagree on which document is the env one. - Cleanups.
LockfileEntry::pnpm_env: bool(vendor/lock_inventory/mod.rs) on the cross-ecosystem entry duplicatesTreePart.PnpmLockState::materialize()(formats/pnpm/hosted.rs) clears the vendored cache when it doesn't need to.is_rushis probed twice per inventory (vendor/lock_inventory/npm_family.rsandvendor/lock_inventory/pnpm.rs).
Context: the burn-down review comment on #2009.
🤖 Generated with Claude Code
Activity
mikolalysenko commentedon Nov 9, 2026
Update from the
/code-review highpass on #2009 after the withhold change:UnwiredCopyscope vsUnattestedkey item.drop_env_twinsnow runs before any contest. An env twin of a project ref therefore leaves noUnattestedmark and no diagnostic that could withhold the project ref.is_rushwas probed twice per inventory. The Rush reader no longer statsrush.jsonagain.InstalledPackages::from_lock_text(crates/socket-patch-core/src/formats/pnpm/mod.rs) now reads keys throughgrammar::entries, which accepts only key lines that end in:. The old walk also accepted an inlinename@1.0.0: {}packages entry. pnpm never writes a packages entry without aresolution:body, so this is theoretical.PnpmLock::parse_keys(the store crawler's hot path) still runsgrammar::documentsand tags every entry by document, butInstalledPackagesnever reads the tag. That costs about two extra whole-text passes per current lockfile compared with the old single walk. A path that skips document tagging would remove them.🤖 Generated with Claude Code
mikolalysenko commentedon Nov 9, 2026
Open question from #2009: an env-document (config-dependency) ref is withheld when the project document of the same lock resolves the release elsewhere, but another lock (say
package-lock.json) resolving it from the registry still doesn't withhold it, becausecontest_across_locksexempts the env tree both ways. Should that reasoning (a VEX statement covers the release, not one tree) extend across locks too?