[Mikola Lysenko (mikolalysenko)]

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

Fixes #1937

Summary

After vendoring an npm lockfileVersion 2 project, a plain npm install (npm 7-12) re-saves the lock and drops resolved from the legacy dependencies mirror node. rollback, remove and vendor --revert then half-reverted the lock and kept the vendored state. With this PR they restore both halves of the lock and delete the artifact. When something really did drift, they leave both halves alone, so the lock always stays consistent with itself.

Root cause

revert_one_record (crates/socket-patch-core/src/vendor/npm_lock.rs) only treated a live fragment as "ours" when its resolved was the vendored one. npm never writes resolved for a file: resolution in the v2 mirror, so the node keeps only version and the patched integrity. #879 taught vex and vendor --check that this shape is still wired, but the revert engine still called it drift. Each record was also reverted on its own, so the packages entry was restored to the registry while its mirror twin was kept. npm 6 then failed EINTEGRITY, and the run said "vendored state left untouched".

The vendor side had the same blind spot (found in review). Re-running vendor/scan, which was the printed remedy, treated the resolved-less node as not vendored and recorded the patched node as its pre-vendor original. A later revert would put the patched integrity back.

Changes

• Revert ownership: a legacy mirror node with no resolved and the recorded vendored integrity counts as ours (legacy_node_lost_our_resolved, the Vendored npm vex and vendor --check fail after any npm 7–10 npm install on a lockfileVersion 2 lock, because npm drops resolved from the legacy mirror and #813 treats that as an unpatched npm 6 install (regression) #879 rule).
• Twin gate: a dry pass (TwinGate) finds packages with a drift-kept record. Their other records that would be rewritten are left alone, with a vendor_lock_entry_drifted warning naming the drifted twin. A converged, removed or moved-off twin is not blamed. Locks with no mirror records (v3, v1-free) skip the dry pass.
• Vendor: a resolved-less mirror node carrying our integrity counts as already vendored. Its record gets original: None, which carry_forward_wiring fills from the previous record.

Tests (red → green)

Test	Without fix	With fix
vendor::npm_lock::tests::v2_revert_restores_a_resolved_less_mirror_node_with_our_integrity	drift warning, artifact kept	lock byte-identical to pre-vendor, artifact gone
vendor::npm_lock::tests::v2_revert_keeps_the_packages_twin_of_a_drifted_mirror_node	packages entry reverted (half-revert)	lock untouched
vendor::npm_lock::tests::v2_revert_keeps_the_packages_twin_of_a_re_resolved_mirror_node	packages entry reverted	packages stays wired
vendor::npm_lock::tests::v2_revendor_of_a_resolved_less_mirror_node_records_no_original	original = patched node	original: None
vendor::npm_lock::tests::v2_revert_does_not_blame_a_converged_packages_twin	(new gate behavior)	one drift warning, for the mirror only
e2e e2e_vendor_npm_build::npm_v2_revert_after_npm_install_restores_both_lock_halves (real npm 10.9.9, --lockfile-version 2; rollback, remove <purl>, vendor --revert, re-vendor + vendor --revert)	rollback exits 1 rollback_failed; re-vendor case leaves the mirror on the patched integrity	all exit 0, both halves restored, artifact removed

Commands run locally:

• cargo fmt --all -- --check: clean
• cargo clippy --workspace --all-features -- -D warnings: clean
• cargo test -p socket-patch-core --all-features --lib vendor::npm_lock: 122 passed
• cargo test -p socket-patch-cli --all-features --test e2e_vendor_npm_build -- npm_v2_revert_after_npm_install: passed. Red on the base code: rollback_failed; red without the vendor-side change: the mirror node was left on the patched integrity.
• cargo test -p socket-patch-core --all-features: 7000 passed, 4 failed. The 4 also fail on main in this container, because it runs as root and root ignores read-only and unremovable permissions: pypi_poetry::wire_write_failure_maps_error_and_leaves_lock_untouched, pypi_requirements::wire_failure_rolls_back_already_written_files, copy_tree::relax_loop_must_not_traverse_symlinked_root, vlt_heal::an_unremovable_hidden_lock_keeps_every_store_entry.
• cargo test -p socket-patch-cli --all-features --lib --test e2e_vendor_npm_build: 940 + 44 passed
• cargo test --workspace --all-features could not finish locally: building every integration-test binary ran out of this session's disk allowance (No space left on device). CI runs the full matrix.

Per-issue checklist:

• Vendored npm rollback / remove / vendor --revert on a lockfileVersion 2 lock fail after any npm install, yet half-revert the lock: packages goes back to the registry while the legacy mirror keeps the patched integrity, and npm 6 then fails EINTEGRITY #1937: rollback / remove / vendor --revert after npm install on a v2 lock (e2e plus unit test); no half-revert when one half really drifted (two unit tests); re-vendor remedy path (unit plus e2e).

Review notes (/code-review high), not changed

• SRI comparison: the new rule compares integrity as an exact string. That is what Vendored npm vex and vendor --check fail after any npm 7–10 npm install on a lockfileVersion 2 lock, because npm drops resolved from the legacy mirror and #813 treats that as an unpatched npm 6 install (regression) #879's vex rule does too (pins_a_wired_ref compares the SRI strings exactly), so revert and vex agree.
• Workspace members' mirror pairing: legacy_pointer_packages_key maps /dependencies/ws/dependencies/x to node_modules/ws/node_modules/x, not packages/ws/node_modules/x. lock_inventory uses the same pre-existing convention. For a nested dependency of a workspace member, the twin gate falls back to the previous per-record behavior: no regression, but no gate either. Possible follow-up.
• Wiring the gate to the warning code: the gate keys on the vendor_lock_entry_drifted code. That is the same code RevertOutcome::drift_skipped uses to keep state, so the two can't disagree.

Wrappers

Only Rust core and CLI tests change. The npm, PyPI and gem wrappers only dispatch the binary.

https://claude.ai/code/session_011f9aeNkhAFT7jRyt8GURGB

---

Note

Medium Risk
Changes npm lock revert and re-vendor wiring logic on a critical path; behavior is heavily tested but incorrect twin pairing could still leave locks inconsistent in edge cases.

Overview
Fixes half-reverts of npm lockfileVersion 2 locks after a normal npm install (npm 7–12), which strips resolved from the legacy dependencies mirror while leaving the vendored integrity.

Revert now treats a mirror node with no resolved but the recorded vendored integrity as still ours (same rule as vex / vendor --check, #879), so rollback, remove, and vendor --revert can restore both the packages entry and its mirror and remove the artifact. A TwinGate dry-run skips rewriting one half when the other half would be drift-kept, avoiding a self-inconsistent lock (npm 6 EINTEGRITY).

Re-vendor treats that mirror shape as already wired and does not snapshot the patched node as original; carry_forward_wiring keeps the true pre-vendor state.

Adds focused unit tests in npm_lock.rs and an e2e case exercising rollback, remove, revert, and re-vendor after npm install.

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

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

bugbot run

---

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 f6f0ee6. Configure here.}

[Mikola Lysenko (mikolalysenko)]

Ready for review — burn-down agent.

• Head: f6f0ee669789151fd730556d451e2a336e0991e2
• CI: all 82 check runs on this head succeeded or were skipped. None failed.
• Bugbot reviewed f6f0ee66 and found no issues.
• Mergeable with no conflicts. The PR doesn't touch CHANGELOG.md.
• For the reviewer: the change is in vendor/npm_lock.rs, where reverting an npm v2 lockfile now restores both the packages and dependencies mirrors. The new e2e coverage is in e2e_vendor_npm_build.rs. Not announced in Slack yet: the send tool wasn't available in this run, so a later run will retry.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

Final review brief

What it does: After vendoring an npm lockfileVersion 2 project, a plain npm install (npm 7-12) drops resolved from the legacy dependencies mirror node. rollback, remove and vendor --revert then reverted the packages entry but kept the mirror, so the lock disagreed with itself. This PR counts a resolved-less mirror node that still has our integrity as ours (the same rule #879 gave vex), restores both halves together through a dry-pass "twin gate", and stops re-vendor from recording the patched node as the pre-vendor original.

Risk: medium. It changes the npm lock revert path, which deletes artifacts. The new ownership rule is narrow (an exact integrity match), and the twin gate only holds records back, never forward, so when it is wrong it keeps the vendored state rather than losing data.

Look here:

• npm_lock.rs:1483-1496: revert treats a resolved-less mirror node with our integrity as ours
• npm_lock.rs:1340-1400: TwinGate dry pass and kept_by_twin
• npm_lock.rs:830-848: where the gate is wired into revert_npm_opts
• npm_lock.rs:1704-1724: vendor side, original: None for an already-vendored mirror node
• e2e_vendor_npm_build.rs: real npm 10 e2e of rollback, remove, revert and re-vendor after npm install

Verified:

• Read the full diff. CHANGELOG.md is untouched, there is no debug code, and the only refactor is the small record_packages_key helper.
• cargo test -p socket-patch-core --all-features --lib vendor::npm_lock: all of the PR's tests pass.
• Four of the five new unit tests exercise paths the fix changes. The fifth, v2_revert_does_not_blame_a_converged_packages_twin, guards the new gate.
• CI: 31 checks passed, 20 skipped, 0 failed (ci-ok and clippy green). Cursor Bugbot passed. No open review threads.

Changes I made: none.

Open questions (not blocking, follow-up material):

1. Patch upgrade after npm install. Re-vendoring with a new patch whose integrity differs (npm_lock.rs:1708-1709 compares against the new self.integrity) still records the old patched mirror node as original. A later revert then restores the patched integrity into the mirror without a warning. This behavior predates the PR, and the PR fixes the same-patch re-vendor remedy it prints, but the new-integrity case is worth a follow-up issue. A possible check: the mirror's twin pointed into the vendor dir before the rewrite, and the node's integrity equals that twin's pre-rewrite integrity.
2. Workspace-nested twins. legacy_pointer_packages_key maps them to node_modules/ws/node_modules/x, not packages/ws/.... Those pairs fall back to the old per-record behavior. That is no regression, but it is not gated either (the PR description already notes this).

Auto-merge (squash) is armed, so approving sends this straight to the merge queue.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

[agent] On the final review brief's open questions: both stay out of this PR, as follow-ups.

1. Re-vendoring with a different patch after npm install. This is real. The mirror node carries the previous patch's integrity, which differs from the new one, so legacy_node_lost_our_resolved misses it, and the old patched node is recorded as original. It predates this PR and needs a separate signal: the twin packages entry pointed into .socket/vendor/ before the rewrite, and the node's integrity equals the twin's integrity before the rewrite. Fixing it here would widen a change that is already reviewed. Vendored npm rollback / remove / vendor --revert on a lockfileVersion 2 lock fail after any npm install, yet half-revert the lock: packages goes back to the registry while the legacy mirror keeps the patched integrity, and npm 6 then fails EINTEGRITY #1937's reported path is a same-patch re-vendor, and that is covered and tested here.
2. Workspace-nested twins. This is the existing legacy_pointer_packages_key convention, shared with lock_inventory. Those pairs keep the old per-record behavior, so nothing regresses.

---

Generated by Claude Code

[Wenxin Jiang (Wenxin-Jiang)]

[agent] Reviewed f6f0ee669789151fd730556d451e2a336e0991e2. Checked atomic npm v2 twin restoration and ownership after npm drops legacy resolved fields. No major correctness issues found. Validation: source and regression-test review plus the available exact-head CI checks; no local test pass is claimed.