Skip to content

Fix npm-family restore ignoring project registry (#908, #521) - #918

Open
Mikola Lysenko (mikolalysenko) wants to merge 6 commits into
mainfrom
agent/fix-npm-restore-project-registry
Open

Mikola Lysenko (mikolalysenko) wants to merge 6 commits into
mainfrom
agent/fix-npm-restore-project-registry

Conversation

@mikolalysenko

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

Copy link
Copy Markdown
Collaborator

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

Fixes #908
Fixes #521

Summary

Hosted rollback / remove (and the hosted → vendored takeover) restore
a yarn berry or vlt lock entry from the npm version document. That document
was always read from the default registry, never from the registry the
project installs from. On a mirror whose tarball URLs are off the usual
<registry>/<name>/-/<leaf>-<ver>.tgz path, the restored lock then pointed
yarn / vlt at a URL the mirror never serves, so cold-cache installs 404'd
while socket-patch reported success.

Root cause (shared)

upstream::npm::fetch_dists → UpstreamClient::npm_dist only ever asked
npm_registry_base() (SOCKET_NPM_REGISTRY or registry.npmjs.org):

Fix

  • UpstreamClient::npm_dist_on(base, name, version) reads the document
    from a given registry. The cache is keyed per registry, and npm_dist
    is npm_dist_on(npm_registry_base(), …).
  • fetch_dists_on(wanted, registry, …) reads each document from the
    registry the project resolves the package against. npmjs, the
    registry.yarnpkg.com alias and SOCKET_NPM_REGISTRY count as the
    default, so behavior there is unchanged. If the project's registry can't
    be read (e.g. a private mirror that wants credentials the restore doesn't
    send), it falls back to the default registry's document, as before, and
    warns with the new additive code upstream_registry_fallback.
  • Berry passes .yarnrc.yml npmRegistryServer. vlt passes the node's
    registry_base and writes slot [3] from that registry's dist.tarball.
    The lock's slot-[3] presence convention is unchanged.
  • fetch_dists (package-lock, pnpm, bun, classic) keeps the default
    lookup.
  • CLI_CONTRACT.md documents the lookup and the new warning code.

Test evidence

Each regression test fails on main (9c43dfc) and passes on this branch:

Issue Test main branch
#908 in_process_redirect::yarn_berry_rollback_reads_the_tarball_from_the_project_registry (mirror via npmRegistryServer, CDN dist.tarball, default registry serving conventional URLs; expects the ::__archiveUrl= binding back byte for byte) FAILED (bare locator) ok
#521 upstream::vlt::tests::restore_takes_slot3_from_the_node_registrys_dist_tarball (node registry advertises /_cdn/files/…; expects that URL and the node registry's integrity) FAILED (synthesized /mirror/left-pad/-/left-pad-1.3.0.tgz, default registry's integrity) ok
fallback upstream::vlt::tests::unreadable_node_registry_falls_back_with_a_warning n/a ok
helpers upstream::npm::tests::berry_reads_the_project_registry_except_for_npm_scopes, npmjs_and_its_yarnpkg_alias_are_the_default_registry n/a ok

Local runs:

  • cargo test -p socket-patch-core --all-features --lib upstream::: 87 passed
  • cargo test -p socket-patch-cli --all-features --test in_process_redirect yarn_berry: 8 passed
  • cargo clippy --workspace --all-features -- -D warnings: clean
  • rustfmt --check on the touched files: clean. main itself isn't
    cargo fmt-clean (about 120 files) and CI has no fmt gate, so this PR
    formats only what it touches.
  • cargo test -p socket-patch-core --all-features --lib: 5249 passed and 5
    failed. None of the failures are in this PR's code, and all 5 also fail
    on main: utils::digest::tests::production_digests_go_through_the_helpers
    (Gradle/JVM files that hash inline; Route Gradle digests through utils::digest #878 fixes it), plus four
    permission-based tests (copy_tree symlinked root, vlt_heal unremovable
    lock, poetry/requirements write failure) that can't fail a write when
    the sandbox runs as root.
  • The full cargo test --workspace --all-features build used up this
    session's disk allowance (ENOSPC while linking), so CI runs the remaining
    CLI suites.

Ported main fix

main (9c43dfc) is red on coverage / test (macos|windows) because of
utils::digest::tests::production_digests_go_through_the_helpers. This PR
carries #878's three-file fix (48798c4), which becomes a no-op once #878
merges.

Follow-ups (not in this PR)

  • A scoped package under a .yarnrc.yml npmScopes block keeps the
    default-registry lookup. There's no nested YAML reader in core yet.
  • The package-lock / pnpm / bun restores still read the default registry's
    dist.tarball. They aren't reported broken, but they have the same
    shape if a project's .npmrc registry= mirror uses off-path URLs.

🤖 Generated with Claude Code


Note

Medium Risk
Changes hosted unwind registry I/O and lock rewriting for Yarn Berry and vlt; incorrect mirror handling could still affect installs when fallback triggers, but default-registry projects are unchanged and failures remain explicit via warnings or refusals.

Overview
Hosted rollback/remove (and hosted→vendored takeover) used to resolve npm-family lock entries only from the default registry (SOCKET_NPM_REGISTRY / npmjs). That broke projects on mirrors whose dist.tarball URLs differ from the conventional path: Yarn Berry lost ::__archiveUrl= bindings (#908) and vlt slot [3] was synthesized instead of using the mirror’s URL (#521).

The restore path now fetches each package’s version document from the registry the project resolves against—Yarn Berry via .yarnrc.yml npmRegistryServer (with a scoped-package caveat when npmScopes is present), vlt via each node’s registry base and that document’s dist.tarball for slot [3]. UpstreamClient::npm_dist_on and a per-registry cache back this; package-lock/pnpm/bun/classic still use the default lookup via fetch_dists. If the project registry can’t be read, behavior falls back to the default document and emits the additive warning upstream_registry_fallback.

CLI_CONTRACT.md documents the lookup rules and the new warning. An integration test covers Berry rollback with a mirror; unit tests cover vlt slot [3], fallback, and registry helpers. Touched Gradle/JVM/Maven code paths now use shared utils::digest helpers (aligned with #878).

Reviewed by Cursor Bugbot for commit 9c52af9. Configure here.


Generated by Claude Code

Assisted-by: Claude Code:claude-opus-5-5
Hosted rollback and remove looked up a package's version document on
the default registry (npmjs or SOCKET_NPM_REGISTRY) only. On a project
that installs from a mirror whose tarball URLs are off the usual path,
that broke the restored lock:

- yarn berry wrote a bare npm: locator, so a cold-cache install asked
  the mirror for a path it never serves and failed with a 404 (#908).
- vlt rebuilt slot [3] as <registry>/<name>/-/<leaf>-<ver>.tgz instead
  of the URL the registry advertises, which vlt ci can 404 on (#521).

The restore now reads the document from the registry the project
resolves the package against (.yarnrc.yml npmRegistryServer, the vlt
node's registry) and vlt takes slot [3] from its dist.tarball. If that
registry can't be read (for example it needs credentials), the old
default-registry lookup is used and upstream_registry_fallback warns.

Fixes #908
Fixes #521

Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko
Mikola Lysenko (mikolalysenko) force-pushed the agent/fix-npm-restore-project-registry branch from 77ed2e7 to 21413f1 Compare October 6, 2026 06:50
Keeps clippy's type_complexity lint quiet for the restore client's
registry-keyed version-document cache.

Assisted-by: Claude Code:claude-opus-5-5
A scoped package in a .yarnrc.yml with an npmScopes block may resolve
against its scope's registry rather than npmRegistryServer, so berry
restore keeps reading its document from the default registry, as
before, instead of asking a registry that may not host it.

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

Copy link
Copy Markdown
Collaborator Author

BugBot review


Generated by Claude Code

@mikolalysenko
Mikola Lysenko (mikolalysenko) marked this pull request as ready for review October 6, 2026 07:08
main's test suite is red: the Gradle cache, jar and Maven sidecar code
from #646 hashes inline, which the digest guard test from #865 forbids,
so coverage and the macOS/Windows test jobs fail on every PR. This is
the same change as #878, ported so this PR can go green; it no-ops
once #878 lands.

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

Copy link
Copy Markdown
Collaborator Author

[agent] coverage and test (macos-latest) failed on 688d77c. The cause isn't this PR: utils::digest::tests::production_digests_go_through_the_helpers is red on main (9c43dfc) because the Gradle cache, jar and Maven sidecar code (#646) hashes inline, which the guard added in #865 forbids. main's own CI run fails the same jobs. I ported #878's fix (the same 3-file change) in 48798c4. It becomes a no-op once #878 merges.


Generated by Claude Code

@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.

Stale Bugbot comment from a previous run.

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[agent] test (windows-latest) fails on 48798c4. It failed on the first run and again on one re-run (job 112171077636). macOS, coverage, test-release, the ecosystem compatibility workflows and Bugbot are all green on this head. The PDM 2.29.2 leg failed once and passed on re-run.

Blocked: I can't read the job log. The agent environment's network policy denies the Actions log host (productionresultssa*.blob.core.windows.net), and the check run has no annotations beyond the exit code. Nothing in this diff is OS-specific. My best guess: restore now contacts the registry a lock or .yarnrc.yml names before falling back, so a test whose fixture names an unreachable registry (*.example, or http://127.0.0.1:1) now does a network round trip. On Windows that may behave differently, e.g. a slow connect-refused, or an extra upstream_registry_fallback warning in an asserted envelope.

Needed: the name of the failing test (or the log tail from the Run tests step), or the blob host added to the agent environment's allowed domains. I'll fix it from there.


Generated by Claude Code

The npm dist cache is now keyed by registry base, and these two tests
seed it under npm_registry_base(), which reads SOCKET_NPM_REGISTRY.
Serial vlt/bun tests set that variable, so when one ran in parallel the
lookup key no longer matched the seeded entry and the test fetched
left-pad from the other test's mock server (404). That is the
test (windows-latest) failure on 48798c4. Serializing them with the
env-mutating tests closes the race.

Co-Authored-By: Claude <noreply@anthropic.com>
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[agent] Unblocked: the GitHub MCP can read the job log. test (windows-latest) on 48798c4 failed in patch::redirect::upstream::client::tests::berry_metadata_is_registry_anchored_without_repacking with GET http://127.0.0.1:60608/left-pad/1.3.0: HTTP 404.

Root cause: this PR keys the npm dist cache by registry base. The test seeds that cache under npm_registry_base(), which reads SOCKET_NPM_REGISTRY. Serial vlt/bun tests set that variable while this test ran in parallel, so the lookup missed the seeded entry and fetched from the other test's mock server. 9c52af9 marks both berry checksum tests #[serial_test::serial]. Locally, the upstream tests pass (90/90) and clippy --lib -D warnings is clean.


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

bugbot run


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 9c52af9. Configure here.

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

Copy link
Copy Markdown
Collaborator Author

Burn-down agent: labeled Ready for review at 9c52af9 (9c52af9fefe75059c55fc9172605c0c4a5b4393d).

  • CI: 544 check runs on 9c52af9: 541 success, 3 skipped, 0 failing, none pending. Mergeable against main (0 behind).
  • Bugbot: reviewed 9c52af9 and found no new issues. No review threads are open.
  • For the reviewer: 9c52af9 serializes the two berry checksum tests (#[serial_test::serial]), because the dist cache is now keyed by the registry base that SOCKET_NPM_REGISTRY sets.

Generated by Claude Code

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

3 participants