GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Fix vendored revert keeping artifact for removed lock entry (#665) by mikolalysenko · Pull Request #689 · SocketDev/socket-patch

Conversation

Mikola Lysenko (mikolalysenko) commented Oct 3, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Start fix for #665

e8b2389

Assisted-by: Claude Code:claude-opus-5-5

This was referenced Oct 3, 2026

After yarn remove of a vendored package, rollback fails forever (exit 1) and no command can clean up the orphaned yarn classic artifact; the remedies it prints don't work #665

Closed

Vendored yarn classic rollback can't undo a block yarn has merged with another range (left-pad@^1.1.0, left-pad@^1.3.0:): it reports the block as gone, exits 1 forever and leaves the lock wired #692

Closed


          Drop vendored artifact after dependency removal

8f841c0

After `npm uninstall`, `yarn remove`, `pnpm remove` or `bun remove`
of a vendored package, the revert treated the vanished lock entry as
drift. It kept the artifact and ledger entry forever, so `rollback`
and `remove` exited 1 on every run, `scan --prune` kept the entry,
and the printed remedies looped.

A vanished entry now warns `vendor_lock_entry_removed` instead of
`vendor_lock_entry_drifted`. The artifact and entry are dropped once
no wired file still mentions the uuid dir, and are kept, as before,
while one does. Re-resolved entries are still drift-kept.

Fixes #665

Assisted-by: Claude Code:claude-opus-5-5

Mikola Lysenko (mikolalysenko) force-pushed the agent/fix-vendor-revert-removed-lock-entry branch from a64e33a to 8f841c0 Compare

October 3, 2026 12:51


          Expect prune to reclaim removed vendored deps

6bad4f9

The two scan --prune e2e tests encoded the old behavior that an
uninstalled vendored dependency is drift-kept forever. With #665 the
prune now reverts it in one run, so the tests assert that instead and
check the unwired warning before the prune.

Assisted-by: Claude Code:claude-opus-5-5

Mikola Lysenko (mikolalysenko) marked this pull request as ready for review

October 3, 2026 13:12

Mikola Lysenko (mikolalysenko) commented Oct 3, 2026

Copy link

Copy Markdown

Collaborator Author

BugBot review

Generated by Claude Code

cursor[bot] reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cursor Bot reviewed

Oct 3, 2026

View reviewed changes

cursor Bot left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label

Oct 3, 2026

Mikola Lysenko (mikolalysenko) commented Oct 3, 2026

Copy link

Copy Markdown

Collaborator Author

[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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Mikola Lysenko (mikolalysenko) commented Oct 3, 2026

Copy link

Copy Markdown

Collaborator Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Mikola Lysenko (mikolalysenko) removed the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label

Oct 3, 2026


          Require proof before deleting a removed entry's artifact

43a6967

The #665 gate deleted a vendored artifact once one readable lockfile
lacked the literal `.socket/vendor/npm/<uuid>/` path. That missed a
lock whose resolution uses JSON-escaped slashes, and it ignored an
alternate lockfile (npm-shrinkwrap.json) that exists but cannot be
read, so the next fresh install failed with ENOENT.

Deletion now needs every existing wired file to be read and to not
mention the uuid in any spelling: only NotFound counts as absent,
the match is on the uuid case-insensitively, JSON is parsed so \u
escapes are decoded, and text with an escape the scan cannot see
fails closed.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01BY1gbCU7vLkvqCmxRY9FF4

Mikola Lysenko (mikolalysenko) commented Oct 3, 2026

Copy link

Copy Markdown

Collaborator Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Mikola Lysenko (mikolalysenko) commented Oct 3, 2026

Copy link

Copy Markdown

Collaborator Author

BugBot review

Generated by Claude Code

cursor[bot] reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cursor Bot reviewed

Oct 3, 2026

View reviewed changes

cursor Bot left a comment

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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.}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label

Oct 3, 2026

Mikola Lysenko (mikolalysenko) commented Oct 3, 2026

Copy link

Copy Markdown

Collaborator Author

[burn-down agent] Re-labeled Ready for review at 43a6967b1facd18b203bac99f086bf59c5e92028 (the label came off when the Codex P1 fix was pushed).

CI: 402/402 check runs green on this head (6 skipped by path/matrix filters), 0 failing. Mergeable; only human approval outstanding.
Bugbot: reviewed 43a6967; no review threads open.
Reviewer focus: keep_artifact_while_lock_references_it, the absence check added for the Codex P1 (it must not delete an artifact that an install still needs).

Generated by Claude Code

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Tanmay Singla (Tanmay182003) approved these changes

Oct 5, 2026

View reviewed changes

Mikola Lysenko (mikolalysenko) merged commit 8d84061 into main

Oct 5, 2026

409 checks passed

Uh oh!

There was an error while loading. Please reload this page.

Mikola Lysenko (mikolalysenko) deleted the agent/fix-vendor-revert-removed-lock-entry branch

October 5, 2026 11:25

Mikola Lysenko (mikolalysenko) mentioned this pull request

Oct 7, 2026

vendor --check fails a vendored package whose lock is contested by a sibling package-lock.json with "no lockfile or config references .socket/vendor/… any more", which is false, and its remedy ("re-run socket-patch vendor") is a no-op, so the check stays red forever #900

Closed

Mikola Lysenko (mikolalysenko) added a commit that referenced this pull request

Oct 7, 2026


          Fix unwinding a vendored pnpm parent clobbering its vendored child's …

588466a

…ref (#830)

When a vendored package and its own dependency are both vendored (debug
-> ms), vendoring the child rewrites the `ms` ref inside the parent's
rekeyed snapshot (v9) or packages block (lockfile 5.4/6.0), and records
that ref against the parent's block key at that moment.

Two halves of the revert surgery broke on that interplay:

- revert_block / revert_package_block accepted the parent's live block
  (its key is still ours) and spliced the whole pre-vendor original back,
  dropping the child's live `file:` ref. `remove <parent>` then left
  `debug -> ms: 2.1.2` while ms stayed keyed `ms@file:`, a lock every
  pnpm major rejects with ERR_PNPM_LOCKFILE_MISSING_DEPENDENCY.
- revert_snapshot_ref / revert_pkg_dep_ref looked the child's ref up only
  by the recorded parent key. Once the parent was rekeyed (un-vendored, or
  vendored after the child), the record never matched again: a
  `vendor_lock_entry_removed` warning (drift before #689), and in the
  child-first order the child's `file:` ref was left in the parent block.

The restore is now a three-way merge (merge_live_dep_refs): the recorded
original, except a dependencies/optionalDependencies ref whose live value
moved off what this entry wrote keeps the live value. With live == new it
is the original verbatim, so single-entry reverts stay byte-exact. The
ALREADY CONVERGED probes compare modulo those ref values, so a re-run
after a merged restore stays silent. A ref record whose block key is gone
now looks in the block naming the same package under its other key
(registry key <-> vendored `file:` key, matched by tarball leaf in every
dialect's spelling) and restores only a ref that is ours, through one
shared DepRef helper for both dialects.

Every vendor/revert order of the pair now round-trips byte-identically
with no warnings and no kept artifact; `remove <parent>`, `rollback`,
`vendor --revert` and the vendored -> hosted takeover are covered end to
end through the binary.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>

This was referenced Oct 8, 2026

After npm install <pkg>@<other version> moves a vendored npm package off its patched version, scan --prune, vendor --revert, remove and rollback all drift-keep it, so vendor --check stays red and every remedy it names loops #1155

Closed

After yarn remove of a hosted-pinned yarn berry package, its leftover resolutions pin makes rollback, remove and list fail forever with hosted_wiring_contested, and the remedy they print (re-run the hosted scan) changes nothing #1203

Closed

Reviewers

cursor[bot]

cursor[bot] left review comments

Tanmay Singla (Tanmay182003)

Tanmay182003 approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

Ready for review

Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

After yarn remove of a vendored package, rollback fails forever (exit 1) and no command can clean up the orphaned yarn classic artifact; the remedies it prints don't work

Uh oh!

There was an error while loading. Please reload this page.

3 participants

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.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

Mikola Lysenko (mikolalysenko) commented Oct 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Summary

Root cause

Fix

Test evidence

Uh oh!

Mikola Lysenko (mikolalysenko) commented Oct 3, 2026

Uh oh!

cursor Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mikola Lysenko (mikolalysenko) commented Oct 3, 2026

Uh oh!

Mikola Lysenko (mikolalysenko) commented Oct 3, 2026

Uh oh!

Mikola Lysenko (mikolalysenko) commented Oct 3, 2026

Uh oh!

Mikola Lysenko (mikolalysenko) commented Oct 3, 2026

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Mikola Lysenko (mikolalysenko) commented Oct 3, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Mikola Lysenko (mikolalysenko) commented Oct 3, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

cursor Bot left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.