Skip to content

Stop attesting npm-family pins pnpm or a user patch overrides - #2010

Draft
Mikola Lysenko (mikolalysenko) wants to merge 1 commit into
mainfrom
fix/npm-discovery-false-attestation
Draft

Mikola Lysenko (mikolalysenko) wants to merge 1 commit into
mainfrom
fix/npm-discovery-false-attestation

Conversation

@mikolalysenko

Copy link
Copy Markdown
Collaborator

Problem

VEX discovery (vex/discover/) attested two kinds of npm-family pins that hosted and vendored mode both refuse. In both cases the install may not use the attested bytes.

Fix

  • N1: extract_pnpm runs hosted's own detection, utils::pnpm_workspace::git_branch_locks, through a new git_branch_locks_in(view, env). It follows the same rules as hosted: only outside Rush, and only while pnpm's lockfile setting is on (hosted uses else if there too). When branch locks are in use, every ref of the root pnpm lock (or shrinkwrap.yaml) and of each member's own lock is marked UnattestedKind::PnpmGitBranchLock. This goes through UnwiredCopy { scope: <lock>, target: Any }, so only refs that survive the contests get marked. Hosted and vendored refs are both marked, because vendored mode also refuses this state.
  • N2: hosted's npm Hosted npm scan pins a package that npm 12's native patchedDependencies also patches, so every later npm ci / npm install fails EPATCHFAILED (and vendored refuses the lockfileVersion 4 lock with wrong advice) #711 gate is moved out of rewrite_npm_lock into patch::redirect::NpmUserPatches. It holds the package.json patchedDependencies keys plus the non-null patched records from any present npm lock. The hosted rewriter and npm discovery both use it now (no behaviour change for hosted). Bun discovery uses the rewriters' per-dependency rule, bun_lock_text::patched_dependency_key: for bun.lock it reads the manifest keys and the keys mirrored in the lock, the same as rewrite_bun_lock; for bun.lockb it reads the manifest keys only, the same as the hosted engine and vendor::bun_binary. Each ref is checked against its hosted URL or vendored tarball, so a key naming the pinned tarball itself is still not a conflict. Matching refs are marked UnattestedKind::UserPatchedDependency.
  • CLI: vex_sources.rs gets note arms vex_pnpm_git_branch_lockfile / vex_npm_patched_dependency. CLI_CONTRACT.md documents both: the Unattested references bullet, the code table, and a cross-link from the Hosted scan with pnpm gitBranchLockfile pins the stale pnpm-lock.yaml and reports success, while pnpm installs unpatched bytes from pnpm-lock.<branch>.yaml #556 paragraph.

Why keep the ref and mark it unattested, instead of a diagnostic and no ref

In both cases the writers treat the wiring as present but unchangeable, not as dead:

If the ref were dropped, the ledger liveness checks (hosted_claim / vendored_claim) would report the wiring as reverted. That's the same reasoning as #2005 / #2007, BundledCopy and NpmShrinkwrapOnly.

Choices worth a look

  • Discovery ignores environment variables for gitBranchLockfile. It reads the setting from pnpm-workspace.yaml, .npmrc and the governing ancestor's files only. Hosted and vendored also read npm_config_git_branch_lockfile / pnpm_config_git_branch_lockfile from the environment. I followed discovery's existing precedent for the lockfile setting (lockfile_disabled(view, false): "the hosted engine runs in the install's environment, vex need not"), so vex and the ledger checks don't depend on the scanner's shell.
  • Bun pins whose user patch Bun would drop are not attested either. For a registry-keyed bun patch over an earlier hosted pin, Bun silently drops the user's patch, so the installed bytes may in fact be the pure Socket bytes. I still don't attest them: hosted refuses and never confirms, and the owner rule makes the modes the ground truth.

Tests

Each new test fails on the code before this change (checked by switching the three new marking paths off: all three panic at their unattested assertions) and passes after:

  • vex::discover::npm::tests::pnpm_git_branch_lockfile_marks_the_lock_pins_unattested: root lock via pnpm-workspace.yaml and via .npmrc, plus a sharedWorkspaceLockfile: false member with its own branch lock. Controls: no branch lock, and the setting off.
  • vex::discover::npm::tests::npm_user_patched_dependency_pin_is_not_attested: a manifest key, a v4 lock-entry patched record, a record in the sibling shrinkwrap only, and a key for another package or version as the control.
  • vex::discover::bun::tests::bun_user_patched_dependency_pin_is_not_attested: hosted bun.lock with versioned and bare keys; a key naming the pinned tarball (no conflict); a vendored ref keyed through bun.lock's mirrored keys; hosted bun.lockb.
  • commands::vex_sources::tests::npm_family_unattested_kinds_have_their_own_tags.

Runs:

  • cargo test -j4 -p socket-patch-core --lib -- vex::discover:: patch::redirect:: vendor::npm vendor::bun vendor::pnpm hosted:: utils::pnpm_workspace: 2173 passed.
  • vex::discover::testing::golden::tests::committed_fixture_corpus_matches_golden passes; no golden changes.
  • cargo test -j4 -p socket-patch-cli --lib vex_sources: 15 passed.
  • cargo test -p socket-patch-cli --test in_process_redirect_pnpm: 41 passed.
  • cargo clippy --locked --workspace --all-features -- -D warnings and cargo fmt --all -- --check are clean.

Not done

  • pnpm's own patchedDependencies (pnpm.patchedDependencies / pnpm-workspace.yaml), yarn berry patch: and vlt are not covered. They're out of scope for N2.
  • /code-review high has not been run yet; this PR stays a draft until it is.

🤖 Generated with Claude Code

VEX discovery attested two kinds of npm-family pins that the hosted and
vendored writers refuse, so the statement could cover bytes the install
never uses.

N1, pnpm gitBranchLockfile (#556): with the setting on and a
pnpm-lock.<branch>.yaml present, pnpm installs the branch from that lock.
Hosted withholds the root and member locks and refuses every pnpm pin;
vendored refuses the project. Discovery now runs the same detection
(utils::pnpm_workspace::git_branch_locks, via a new git_branch_locks_in
that skips the environment, as discovery already does for the lockfile
setting) and, only while the lockfile setting is on and outside Rush,
marks every ref of the root and member pnpm locks
UnattestedKind::PnpmGitBranchLock.

N2, user-patched npm / Bun packages (#711, #367): hosted leaves a package
the project patches itself unwired and never confirms it; vendored
refuses. Hosted's npm gate is factored into patch::redirect::NpmUserPatches
(package.json patchedDependencies keys plus non-null lock-entry `patched`
records in any present npm lock), which the hosted rewriter and npm
discovery now share. Bun discovery applies the rewriters' per-dependency
rule (bun_lock_text::patched_dependency_key over the manifest keys, plus
bun.lock's mirrored keys for the text lock) against each ref's hosted URL
or vendored tarball. Matching refs are marked
UnattestedKind::UserPatchedDependency.

Both keep the ref (rollback / remove / list still find a pre-fix pin and
the ledgers keep the wiring live, since no re-run can clear the evidence)
and only vex omits it, as vex_pnpm_git_branch_lockfile /
vex_npm_patched_dependency. CLI_CONTRACT.md documents both.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Mikola Lysenko (mikolalysenko) added a commit that referenced this pull request Nov 4, 2026
Two follow-ups under the rule "a user patch applied on top means never
attest".

Bun (and npm) keys naming the pinned tarball itself. #2010 kept attesting
a pin whose `patchedDependencies` key names the pin's own tarball (a `bun
patch` made on top of the Socket pin, #1019), because discovery reused
the hosted rewriter's per-dependency rule, where such a key is no
conflict. Bun applies that patch over the Socket bytes, so the install
never runs the Socket patch alone. Discovery now reads the keys against
no target (any key for the name@version counts) and marks such a ref
UserPatchedDependency with its own detail; npm discovery reads its keys
the same way. bun_user_patched_dependency_pin_is_not_attested pinned the
old attested behavior and now asserts the mark. The hosted rewriter is
unchanged: it keeps such a pin (and confirms it on an in-sync re-run),
while vex's plan omits it through the unattested mark.

Workspace members. The consent gate walked workspace members and
ancestor workspace settings; discovery and the in-memory engine read the
project root only. UserPatchIndex's walk now runs over a ProjectView and
is shared: discover() (the gate) is the walk over the disk with every
ecosystem, npm_family_in() the same walk over any view with the
npm-family declarations (pnpm, yarn, patch-package), so members'
package.json pnpm.patchedDependencies, yarn patch: descriptors, patches/
folders and --patch-dir, and their pnpm/yarn files are read alike. The
member glob walk is rewritten over the view (same depth, skip list,
limits and link handling as the walkdir one), and an ancestor with any
workspace marker is walked in both scopes.

The walk never reads outside the git repository root (the nearest
ancestor holding .git, inclusive; in memory the project itself): the
ancestor walk stops there, as before, and a workspace member or literal
--patch-dir beyond it now protects every npm package with an "outside
the git repository" diagnostic instead of being read, in the gate too.
Without a .git the gate's home-directory rule still bounds the ancestor
walk. workspace_member_patches_agree_with_the_consent_gate checks
discovery and the gate agree on member cases, on disk and in memory, and
on a member beyond the repository root.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant