Fix vendored revert keeping artifact for removed lock entry (#665)

[Mikola Lysenko (mikolalysenko)]

LLM Description written by Claude Code:claude-opus-5-5

Fixes #665

Summary

After yarn remove, npm uninstall, pnpm remove or bun remove of a vendored package, rollback and remove exited 1 forever and scan --prune kept the entry. vendor --revert reported success without cleaning anything up, and the remedies the CLI printed looped. With this change the first rollback / remove / vendor --revert / scan --prune drops the unreferenced artifact and ledger entry, leaves the user's lock byte-identical, and says so with a warning. Later runs are clean exit-0 no-ops.

Root cause

The npm-family vendored reverts handled a recorded lock entry that no longer exists the same way as one a third party re-resolved: they emitted vendor_lock_entry_drifted. The affected reverts are:

• yarn classic and berry, via the shared revert_recorded_block
• npm and bun, via revert_one_record
• pnpm v9 and legacy, via the importer, packages, snapshot and dep-ref reverts

RevertOutcome::drift_skipped() keys on that code, so each backend returned early with keep_artifact before anything checked whether the lock still resolved through the artifact. The issue covers yarn classic; the npm comment on #665 confirms npm. Bun and pnpm share the same code path.

Fix

• A vanished entry now warns with a new code, vendor_lock_entry_removed (vendor::LOCK_ENTRY_REMOVED_CODE, plus RevertOutcome::lock_entry_removed()). It does not count as drift.
• A new shared gate, npm_flavor::keep_artifact_while_lock_references_it, runs in every npm-family revert before the artifact is deleted.
  • If any wired file still mentions .socket/vendor/npm/<uuid>/, the gate keeps the artifact exactly as before. The npm, yarn classic and berry reverts check their lockfiles; the pnpm revert also checks package.json / pnpm-workspace.yaml. Examples are an entry re-hoisted or re-keyed to a key the wiring never recorded, or an unreadable lock.
  • Only when the uuid dir is proven unreferenced is the artifact removed.
• Deletion requires proof: every wired file that exists must be read (only NotFound counts as absent; an unreadable lock keeps the artifact) and must not mention the uuid in any spelling (case-insensitive, JSON parsed so \u/\/ escapes are decoded; an undecodable escape fails closed).
• Re-resolved entries (the block still exists with different content), unknown kinds, keyless records and missing sections are still drift-kept. The existing fail-closed still-wired probes in npm, yarn classic and berry are unchanged.
• CLI_CONTRACT.md documents the rule next to the existing composer / maven / nuget "a file that no longer references it is warned about and the artifact removed" rule.

vendor --check reporting vendor_check_ok for the removed package (one row of the issue's table) is unchanged. With the fix, the first rollback or prune removes the entry, so there is nothing left for --check to report.

Test evidence

New regression tests fail on main and pass with the fix (red → green):

Scenario	Test	Before	After
#665, yarn classic (yarn remove)	yarn_classic_lock::tests::revert_after_yarn_remove_drops_the_unreferenced_artifact	FAILED (kept)	ok
#665, npm (npm uninstall)	npm_lock::tests::revert_after_dependency_removed_drops_the_unreferenced_artifact	FAILED (kept)	ok
#665, CLI rollback twice + vendor --revert	in_process_vendor::rollback_after_dependency_removed_cleans_up_and_converges	FAILED (exit 1)	ok
#665, bun (bun remove)	bun_lock::tests::vanished_entry_drops_the_unreferenced_artifact	kept	ok
#665, pnpm v9 / legacy	vanished_rekeyed_packages_block_*, vanished_importer_dep_entry_*, snapshot_ref_*, legacy assert_removed matrix	kept	ok
#665, scan --prune	scan_vendor_e2e::scan_prune_reverts_unused_vendored_entry, scan_vendor_prune_reconciles_unwired_entry_on_an_empty_crawl	kept	reverted

Guard tests check that the artifact is still kept while the lock references it:

• npm_lock::revert_keeps_artifact_when_a_vanished_entry_moved_to_an_unrecorded_key
• yarn_classic_lock::revert_keeps_artifact_when_a_vanished_block_was_rekeyed
• bun_lock::vanished_entry_keeps_the_artifact_while_the_lock_references_it
• legacy pnpm root dep case, still kept

Existing tests that encoded "vanished = drift" were updated to the new contract. Nothing was skipped or ignored.

Commands run locally:

• cargo clippy --workspace --all-features -- -D warnings: clean. --all-targets reports only pre-existing hits in files this PR doesn't touch.
• cargo test --workspace --all-features --no-fail-fast: 214 binaries ok. 12 tests fail only because the sandbox runs as root, so their chmod 0o555 / unremovable-file write-failure setups can't fail a write. They are in repair, redirect, vlt-heal, copy_tree, pypi and covgap_commands_vendor, outside this diff, and CI runs them as non-root.
• cargo test -p socket-patch-cli --test e2e_vendor_npm_build --test e2e_vendor_yarn_classic_dev_flow -- --include-ignored: ok.
• cargo fmt is not applied tree-wide because main itself is not rustfmt-clean. The new code is formatted and only touched hunks are included.
• The npm, pypi and gem wrappers only dispatch to the binary, so they don't change.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BY1gbCU7vLkvqCmxRY9FF4

---

Note

Medium Risk
Changes vendored revert/GC semantics across all npm-family lock backends; incorrect uuid-unreference proof could delete artifacts still needed for installs, though the new gate is explicitly fail-closed on unreadable locks.

Overview
Fixes #665: npm-family vendoring no longer treats a removed lock entry like third-party drift, so rollback, remove, vendor --revert, and scan --prune can actually clean up after npm uninstall / yarn remove / pnpm remove / bun remove.

Vanished wiring now emits vendor_lock_entry_removed (not vendor_lock_entry_drifted), with RevertOutcome::lock_entry_removed() so drift-based artifact retention does not apply. Before deleting .socket/vendor/npm/<uuid>/, keep_artifact_while_lock_references_it requires proof the uuid is absent from every readable wired lock (and related pnpm surfaces)—re-hoisted, re-keyed, escaped, or unreadable locks still keep the artifact fail-closed.

CLI_CONTRACT.md documents the npm-family rule; integration and unit tests expect one-shot cleanup and convergent exit 0 on repeat runs.

^{Reviewed by Cursor Bugbot for commit 43a6967. Configure here.}

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[cursor]

Stale Bugbot comment from a previous run.

[Mikola Lysenko (mikolalysenko)]

[burn-down agent] Labeled Ready for review at 6bad4f905b47f471360c50fa48df53ab4b244467.

• CI: 408/408 check runs green on this head (6 skipped by path/matrix filters), 0 failing.
• Bugbot: reviewed 6bad4f9, no findings; no unresolved review threads.
• Mergeable; only waiting on human approval.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

Codex review of 6bad4f905b47f471360c50fa48df53ab4b244467: changes needed — P1: the new absence check can delete an artifact still required by an install.

keep_artifact_while_lock_references_it in crates/socket-patch-core/src/vendor/npm_flavor.rs allows deletion after a recorded entry vanishes, but its raw-text, any-readable-file check does not prove that every relevant lockfile is unreferenced.

Two independent reproductions confirm this:

• Escaped JSON reference: re-key a vendored npm entry to a valid package alias and encode its existing .socket/vendor/npm/... resolution with JSON-escaped slashes. The literal and escaped locks parse to the same values. With npm 11.19 / Node 24.21, fresh-cache npm ci --offline installs patched bytes from both before cleanup. On this commit, vendor --revert preserves the literal case but deletes the escaped case's tarball and ledger entry at exit 0 while leaving the lock unchanged. The next fresh-cache npm ci fails with ENOENT. A control binary with unchanged main-branch npm revert sources preserves both cases and both installs succeed.
• Unreadable alternate lockfile: when the recorded package-lock.json no longer has the entry, an unrecorded npm-shrinkwrap.json that still names the artifact but is temporarily unreadable is ignored. The readable package-lock makes the helper return Some(false), so the artifact is deleted. Making the alternate lock readable correctly keeps it; an unreadable recorded lock also fails safely.

The new deletion path needs to distinguish absent files from unreadable ones and recognize semantically equivalent references before claiming absence. I’m correcting the shared check and adding regressions. Ready for review is being held off until the correction, native install controls, and fresh CI/review are clear.

[Mikola Lysenko (mikolalysenko)]

Both Codex P1 reproductions are confirmed and fixed in 43a6967. Pushed now so a parallel correction isn't needed. If you already have one, please rebase it onto this or drop it.

keep_artifact_while_lock_references_it now deletes only when uuid_proven_unreferenced holds:

• Unreadable vs absent: only NotFound counts as absent. Any other read error on a wired file (permissions, invalid UTF-8, not a regular file) keeps the artifact, even when another lock is readable and clean.
• Equivalent spellings: the match is on the uuid alone, ASCII case-insensitively, so \/-escaped slashes, backslash separators and case variants all count. .json files are also parsed, and every key and string value is checked, which decodes \u escapes. Text containing a \u/\x/\U escape the raw scan can't see fails closed: YAML, yarn.lock, bun.lock, or JSON that doesn't parse.

Regressions in npm_lock::tests fail on 6bad4f9 and pass now:

• revert_keeps_artifact_when_a_moved_entry_uses_escaped_slashes
• revert_keeps_artifact_when_an_alternate_lock_is_unreadable: the unreadable shrinkwrap is kept. Once it's removed, the same revert reclaims the artifact.

The unreadable case uses invalid UTF-8 rather than file permissions so it also fails as root. The full vendor:: lib suite and the in_process_vendor, scan_vendor_e2e, in_process_rollback_vendored and cli_remove_silent suites pass. The only local failures are the two root-only pypi chmod tests that fail on main too. Clippy is clean. CLI_CONTRACT.md is updated.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 43a6967. Configure here.}