Skip to content

Fix replace-registry-host ${VAR} warning (#1233) - #1404

Merged
Mikola Lysenko (mikolalysenko) merged 3 commits into
mainfrom
agent/fix-npmrc-env-replace-values
Oct 12, 2026
Merged

Mikola Lysenko (mikolalysenko) merged 3 commits into
mainfrom
agent/fix-npmrc-env-replace-values

Conversation

@mikolalysenko

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

Copy link
Copy Markdown
Collaborator

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

Fixes #1233

Summary

npm env-replaces every config value (@npmcli/config parseField → envReplace) in every layer. So replace-registry-host=${RRH} with RRH=always rewrites every hosted pin exactly like =always, and every npm ci then fails E404. The hosted scan read the literal ${RRH}, matched no host, gave no #812 warning and reported success.

The effective replace-registry-host value is now trimmed and env-replaced the way npm does it, in the env, project, user, global and builtin layers. The redirect_npm_replace_registry_host warning fires, and it quotes both the value as written and what npm expands it to, e.g. the project .npmrc sets `replace-registry-host=${RRH}` (npm expands it to `always`). An unset ${RRH} stays literal, as it does in npm, and matches no host.

Root cause

effective_replace_registry_host and outer_file_value passed npmrc_top_level_value(...) through unchanged. Only path-typed settings were env-replaced (NpmConfigEnv::config_path).

Change

  • NpmConfigEnv::config_value trims a value and env-replaces it, the way parseField reads a string. config_path now reuses it.
  • OuterAllowRemote::config_env keeps the environment the layers were resolved with, so the project .npmrc value is expanded against the same environment.
  • effective_replace_registry_host returns EffectiveSetting { value, raw, source }. The engine matches on value, and npm_replace_registry_host_detail shows raw and value.
  • CLI_CONTRACT's replace-registry-host section documents the env-replace.

Test evidence (red → green)

Issue row Test Without fix With fix
project .npmrc =${RRH}, RRH=always CLI vendor::redirect_npm_allow_remote::replace_registry_host_via_env_interpolation_is_warned FAILED (no redirect_npm_replace_registry_host) ok
user .npmrc =${RRH}, RRH=always same CLI test (user leg) FAILED ok
project / user / env layers, ${RRH?}, padded value, unset var core npmrc::tests::replace_registry_host_values_are_env_replaced_like_npm FAILED ok

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: 6302 passed, 4 failed. The 4 are the root-in-sandbox permission tests that also fail on main here (relax_loop_must_not_traverse_symlinked_root, an_unremovable_hidden_lock_keeps_every_store_entry, wire_write_failure_maps_error_and_leaves_lock_untouched, wire_failure_rolls_back_already_written_files).
  • cargo test -p socket-patch-cli --all-features --test vendor -- redirect_npm: 10 passed
  • A full cargo test --workspace ran the sandbox out of disk while linking test binaries. CI runs the whole matrix and is green on 9be0112 (ci-ok success). Bugbot found no issues on 9be0112.

No wrapper changes (npm/, pypi/, gem/) are needed, because the wrappers only dispatch to the binary.

/code-review high

Fixed:

  • The first commit also expanded allow-remote and the vendored allow-file. Review pointed out that expanding a committed ${VAR} against the local environment can turn it into all on one machine and hide a refusal that a CI runner without the variable still hits. Read raw, a ${VAR} already counts as "not all" for those settings, which is the safe direction, so they are back to raw reads. replace-registry-host stays expanded, because expanding it can only add a warning.
  • The warning quotes both the written spelling and the expanded value.
  • config_path reuses config_value.

Not changed:

  • An unset ${RRH} gives no warning, even though CI might set it. npm itself reads it literally in that environment, and guessing at other environments' variables would warn on every templated .npmrc.
  • OuterAllowRemote holds a copy of the env snapshot (config_env). The engine gets the outer layers only through npm_outer and has no other handle on the environment, so this is the smallest way to expand the project layer the same way.
  • Follow-up, out of scope here: other raw .npmrc reads (lockfile-dir, pnpm modules-dir, registry keys) are also env-replaced by the package managers.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TS3s2TugzYrj9UXe71AovS


Generated by Claude Code

Assisted-by: Claude Code:claude-opus-5-5
npm runs every config value through env-replace, in every layer.
socket-patch read replace-registry-host, allow-remote and allow-file
raw, so replace-registry-host=${RRH} with RRH=always looked like the
literal "${RRH}". The hosted scan then gave no warning and reported
success while every npm ci failed with E404 (#1233). The same gap made
allow-remote=${AR} and allow-file=${AF} read as non-"all" values.

Values from the env, project, user, global and builtin layers are now
trimmed and env-replaced before they are compared, like npm's
parseField. An unset ${VAR} stays literal, as it does in npm.

Fixes #1233

Assisted-by: Claude Code:claude-opus-5-5
Review of the first commit: expanding allow-remote and allow-file
against the local environment could flip a committed `${VAR}` to "all"
on one machine and hide a refusal that a CI runner without the variable
still hits. For those two settings the raw `${VAR}` already reads as
"not all", which is the safe direction, so they go back to raw reads.

replace-registry-host stays expanded, because expanding it can only add
the warning. The warning now quotes the value as written and what npm
expands it to, so the user can find the line or variable it names.
config_path reuses the new config_value helper.

Refs #1233

Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko Mikola Lysenko (mikolalysenko) changed the title Fix npm config ${VAR} values in npm guards (#1233) Fix replace-registry-host ${VAR} warning (#1233) Oct 11, 2026
@mikolalysenko
Mikola Lysenko (mikolalysenko) marked this pull request as ready for review October 11, 2026 23:56
@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 9be0112. Configure here.

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

Copy link
Copy Markdown
Collaborator Author

Ready for review (burn-down agent)

  • Head: 9be0112
  • CI: 35/35 non-skipped checks green (50 skipped by path filters, 1 neutral); mergeable, no conflicts.
  • Bugbot: reviewed 9be0112, no findings; no open review threads.
  • Already approved by Wenxin Jiang (@Wenxin-Jiang) on this head. For a reviewer: NpmConfigEnv::config_value now env-replaces every layer's replace-registry-host; an unset ${VAR} intentionally stays literal, matching npm.

Generated by Claude Code

Merged via the queue into main with commit a0987e0 Oct 12, 2026
86 checks passed
@mikolalysenko
Mikola Lysenko (mikolalysenko) deleted the agent/fix-npmrc-env-replace-values branch October 12, 2026 02:57
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