Skip to content

fix: vendor hosted patches directly from lockfiles - #1648

Draft
Mikola Lysenko (mikolalysenko) wants to merge 1 commit into
mainfrom
fix/vendor-lockfile-clean-checkout
Draft

Mikola Lysenko (mikolalysenko) wants to merge 1 commit into
mainfrom
fix/vendor-lockfile-clean-checkout

Conversation

@mikolalysenko

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

Copy link
Copy Markdown
Collaborator

socket-patch vendor could ignore hosted lockfile pins when an agent manifest existed, and an unrelated Python environment could reject a pinned patch as not installed. Select hosted patches from the lockfiles first, download their prebuilt artifacts into .socket/vendor/, and rewrite the dependency files without requiring installed packages.

Reuse the existing lockfile readers and vendoring backend. Hosted conversion skips installed-package discovery and a redundant wiring scan, while reusing the restored-lock preview and failure rollback. Preserve the legacy manifest fallback when no hosted pins are in scope, and update CLI help and documentation.

Validation on current main (dc0dc755d):

  • 334 focused tests passed: 71 vendor unit tests and 263 integration/parser tests covering clean npm/Cargo/Python checkouts, ambient virtualenvs, Bun locks, pnpm refusals, dry runs, revert, and rollback.
  • cargo clippy --workspace --all-features -- -D warnings passed.
  • cargo fmt --all -- --check and git diff --check passed.

Draft pending /code-review high, which AGENTS.md requires before marking a PR ready for review. That command is not available in the publishing session.


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[agent] I ran /code-review high on 25a3728, which AGENTS.md requires before this leaves draft. One finding needs a product decision before this PR can be ready, so I'm leaving it as a draft and labelling it agent:needs-human.

Question for a human: when .socket/manifest.json exists and at least one hosted pin is in scope, should vendor (a) vendor only the hosted lockfile pins, as it does now, (b) vendor the union of the manifest and the hosted pins, or (c) refuse and name the manifest patches it would skip?

As written (vendor.rs:991-1017), vendor returns from run_eject(pins) before the manifest arm runs. Any manifest patch without a hosted pin is skipped silently and the run exits 0. Example: the manifest lists pkg:npm/a@1 applied in place, and package-lock.json pins pkg:npm/b@1 hosted. Before this PR both were vendored; now only b is, and a fresh checkout builds a without its patch. Related outcomes that follow from the same choice:

  • A contested hosted entry now refuses the run (hosted_wiring_contested) before the manifest check. That blocks manifest-only vendoring even when the contested package is not in the manifest.
  • If the manifest and the lockfile pin the same purl at different UUIDs, the selection flips between runs. Run 1 ejects the lockfile UUID; run 2 has no hosted pins left, takes the manifest arm and re-vendors the manifest UUID.
  • --vex after an eject still reads the existing manifest, so manifest patches that aren't installed can turn a successful eject into exit 1.

Other findings for the author, not verified in depth:

  1. run_eject runs the takeover_dry_refusal(..., Some(&service), ...) preview for every pin on wet runs too (vendor.rs:1844). Each preview fetches the archive, so every artifact is granted twice, and a --dry-run that used to make no service requests now downloads every archive.
  2. The eject path never collects vlt_heal::lock_targets, so a vlt project whose .vlt store still holds the hosted bytes no longer gets healed and gets no redirect_vlt_reinstall_required warning.
  3. Installed-package discovery is skipped under eject. A pypi release with two hosted ?artifact_id= variant pins now always fails vendor_variant_ambiguous; before, the installed variant decided it.
  4. takeover_dry_refusal returns None if any file in the shared multi-pin plan has no staged text (for example bun.lockb). One such pin turns off the preview for every pin.
  5. The doc comment on run_eject still describes a per-package takeover that the eject no longer runs.

CI on the current head is green for the draft subset (6 passed, 31 skipped). The full matrix and Bugbot haven't run yet.


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[agent] Burn-down agent: I ran /code-review high on 25a37284 (AGENTS.md requires it before this leaves draft). This needs a product decision before anyone fixes code, so I'm adding agent:needs-human and leaving the draft open.

Question: when .socket/manifest.json exists and the lockfiles have at least one hosted pin, should plain vendor:

  • (a) eject only the hosted pins and stop, as this PR does now, or
  • (b) vendor the union, like main does today: hosted pins from the lockfile, plus manifest patches that have no hosted pin?

With (a), manifest patches that have no hosted pin are skipped. The run still exits 0, so a fresh checkout installs those packages unpatched. Projects that use a manifest also hit new refusals: hosted_wiring_contested, offline_eject_unavailable, and patch_fetch_failed for local-only records.

Confirmed failures (local cargo test -p socket-patch-cli --test mode_migration_cargo on this head: 27 passed, 3 failed):

  • double_takeover_a_b_a_preserves_lock_originals
  • hosted_then_vendored_takeover_leaves_pure_vendored
  • vendor_over_unrestorable_hosted_pin_is_refused

The review also flags the same takeover expectations in mode_migration_npm.rs:621 and e2e_golang_hosted_state.rs:534. CI shows green only because every test job is skipped while the PR is a draft.

Other review findings to handle once (a) or (b) is chosen:

  • The takeover_dry_refusal preflight downloads each prebuilt artifact, including on --dry-run. A wet run then downloads it a second time.
  • The dry-run preview drops PENDING/UNAVAILABLE backend codes. That gives eject_planned on the dry run but eject_incomplete on the wet run.
  • CLI_CONTRACT.md:176 and the run_eject doc comment still describe the per-purl takeover.
  • discover_wiring now runs twice on the manifest path.
  • In vendor_records_reusing, whether EjectCapture is present now quietly turns off discovery, alias resolution and takeover. An explicit flag would be clearer.

Generated by Claude Code

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant