Skip to content

Fix npm v1 lock losing a hosted patch on takeover (#659) - #660

Open
Mikola Lysenko (mikolalysenko) wants to merge 3 commits into
mainfrom
agent/fix-npm-v1-lock-takeover-preflight
Open

Mikola Lysenko (mikolalysenko) wants to merge 3 commits into
mainfrom
agent/fix-npm-v1-lock-takeover-preflight

Conversation

@mikolalysenko

@mikolalysenko Mikola Lysenko (mikolalysenko) commented Oct 3, 2026 •

Copy link
Copy Markdown
Collaborator

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

Fixes #659

Summary

On an npm 6 project (lockfileVersion 1 package-lock.json / npm-shrinkwrap.json), scan --mode vendored / get --mode vendored over a hosted patch un-hosted it and then refused to vendor it, so the project silently went back to unpatched. The refusal now happens before anything is restored: the run still fails with vendor_lockfile_version_unsupported, but the hosted pin and .npmrc stay byte-identical, so the package stays hosted-patched. The dry run previews the refusal instead of "Would download and vendor 1 patch".

Root cause

The hosted→vendored takeover (commands/vendor.rs) restores the upstream registry entry before the npm package-lock backend runs its lockfile-version gate (vendor/npm_lock.rs lock_version_gate). The Bun, vlt, yarn berry and gem backends already raise their lock-text refusals ahead of the restore; npm package-lock did not.

Change

  • Core: npm_lock_vendor_preflight (exported from vendor), the backend's own step-2 gate (parse + v2/v3 check over the lock select_lockfile picks), returning the backend's exact (code, detail). It only answers for package-lock-flavored projects.
  • CLI takeover (commands/vendor.rs): the project-level takeover preflight that was berry-only now also consults the npm package-lock preflight before restore_upstream, dry and wet.
  • Dry-run preview (scan/vendor_flow.rs): scan/get --mode vendored --dry-run lists npm purls of such a project as would_refuse with that code (consistent with the existing Bun/vlt preview rows; does not change the dry-run exit code, per the preview contract).
  • Docs: CLI_CONTRACT.md (takeover reconciliation) and docs/testing/npm-compatibility.md.

Tests

New crates/socket-patch-cli/tests/in_process_vendor_npm_v1_takeover.rs (hermetic: wiremock API and npm registry; the hosted state is produced by a real scan --mode hosted):

Issue variant Test Before fix After
#659 scan --mode vendored, v1 package-lock.json (+ dry-run preview) scan_vendored_over_hosted_v1_package_lock_keeps_the_hosted_pin FAIL (dry run: no refusal preview) pass
#659 get --mode vendored get_vendored_over_hosted_v1_package_lock_keeps_the_hosted_pin FAIL (vendor_takeover_reverted_redirect, pin gone) pass
#659 v1 npm-shrinkwrap.json scan_vendored_over_hosted_v1_shrinkwrap_keeps_the_hosted_pin FAIL (pin gone) pass
control: v2 lock still takes over scan_vendored_over_hosted_v2_package_lock_still_takes_over pass pass

Core unit tests: npm_lock::tests::preflight_matches_the_backend_v1_refusal (same code and detail as the backend) and preflight_passes_v3_and_other_flavors.

Local runs:

  • cargo clippy --workspace --all-features -- -D warnings: clean.
  • cargo fmt --all -- --check: the files this PR touches are clean. main already has unrelated rustfmt drift, and CI doesn't run fmt.
  • cargo test --workspace --all-features: all pass except 12 tests that rely on chmod-based write failures (e.g. covgap_commands_vendor::vendor_state_write_failure_reports_failed_event, repair_invariants::repair_cleanup_failure_…, pypi_poetry::wire_write_failure_…). Those fail because this sandbox runs as root, which bypasses the permission denial. They don't touch this diff; CI runs as non-root.
  • e2e_vendor_npm_build --include-ignored with SOCKET_PATCH_NPM_E2E_REQUIRED=1 (npm 10.9.4): 17/17 pass.
  • e2e_redirect_npm_build: 13/15 pass. The 2 failures are rollback failing to reach https://registry.npmjs.org from this sandbox's HTTP client (error sending request). That code path isn't touched here; CI covers it.

Bugbot: reviewed dcde03b, no issues.


Note

Medium Risk
Changes hosted→vendored takeover ordering for npm projects and lockfile mutation timing, though behavior is stricter (fail earlier) and covered by new integration tests.

Overview
Fixes #659, where scan/get --mode vendored (and vendor) over a hosted npm 6 pin could restore the upstream lock entry first, then fail with vendor_lockfile_version_unsupported because vendoring only supports lockfileVersion 2/3—leaving the project unpatched with the hosted wiring already removed.

The npm package-lock backend now exposes npm_lock_vendor_preflight, mirroring Bun/vlt/Yarn berry: it runs the same v2/v3 (and parse) gate before any takeover restore. Refused runs still exit with the same error code, but package-lock.json / npm-shrinkwrap.json and .npmrc stay unchanged, so the package remains hosted-patched. Vendored dry-run JSON now marks all npm purls in such projects as would_refuse with that code, consistent with other preflight previews.

CLI contract and npm compatibility docs describe the new ordering; hermetic CLI tests cover v1 package-lock, shrinkwrap, dry-run preview, and a v2 control that still completes takeover.

Reviewed by Cursor Bugbot for commit dcde03b. Configure here.


Generated by Claude Code

Assisted-by: Claude Code:claude-opus-5-5
A hosted npm pin on a lockfileVersion 1 lock is lost when
scan/get --mode vendored takes it over: the upstream entry is
restored before the vendored backend refuses the v1 lock (#659).

Assisted-by: Claude Code:claude-opus-5-5
On an npm 6 (lockfileVersion 1) project, scan/get --mode vendored
restored a hosted patch's upstream entry before the vendored backend
refused the v1 lock, so the project silently went back to unpatched.

The takeover now runs the npm package-lock lock gate first, as it
already did for Bun, vlt and yarn berry: the purl is refused with
vendor_lockfile_version_unsupported and stays hosted. The vendored
dry-run preview lists such purls as would_refuse.

Fixes #659

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

Copy link
Copy Markdown
Collaborator Author

BugBot review


Generated by Claude Code

@cursor cursor Bot left a comment

Copy link
Copy Markdown

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 dcde03b. Configure here.

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

Copy link
Copy Markdown
Collaborator Author

[burn-down agent] Ready for review at head dcde03b71449375225e1c9a41df2588e35e1d463.

  • CI: 96/96 green (4 skipped by path filters), mergeable; only waiting on approval.
  • Bugbot: reviewed dcde03b, no findings; no unresolved review threads.
  • Reviewer focus: the new npm package-lock preflight in commands/vendor.rs runs before restore_upstream, so a v1 lock now refuses without touching the hosted pin or .npmrc.

Slack announcement: pending (Slack send tool unavailable in this run; next run retries).


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Codex review of dcde03b71449375225e1c9a41df2588e35e1d463: ready to merge as-is from this review. No actionable findings.

The new preflight uses the backend's own lock selection, JSON parser and version/shape gate, including shrinkwrap precedence. It refuses unsupported locks before scan/get or manifest-based vendor removes the hosted pin. Supported takeover and other package-manager routing remain intact.

Validation:

  • 149 repository tests passed: 118 npm backend/flavor tests, four new v1 takeover tests, 17 preview/planning tests, seven neighboring takeover tests and three Bun controls.
  • Five additional reviewer tests passed 25 scenarios, checking exact scan/get dry-run refusal rows and exit codes, byte-preserving direct vendor refusal, malformed lock shapes, dual-lock priority, other-flavor routing and the six native fixtures.
  • Independent native controls used npm 6.14.18, 8.19.4 and 10.9.9 to generate v1/v2/v3 locks and verify shrinkwrap selection with actual installs. The public Rust preflight agreed with all six fixtures.
  • All seven reviewed source hashes match this commit; independent review and the merge check against main 045d7ec7 are clear.

The separate manifestless vendor path retains its existing behavior: a wet refusal rolls back the hosted files, and its dry run checks the restore plan. Those unchanged boundaries were also verified.

Fresh CI is clear: 485 successful checks, 7 skipped; 13 successful workflows and 1 skipped. Bugbot is clear on this exact commit, with no unresolved threads or outstanding actionable feedback. GitHub's normal human approval requirement remains before merge.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

2 participants