Skip to content
This repository was archived by the owner on May 14, 2026. It is now read-only.

feat: auth#337

Merged
zkochan merged 41 commits into
mainfrom
claude/support-proper-auth-8NRX6
May 13, 2026
Merged

feat: auth#337
zkochan merged 41 commits into
mainfrom
claude/support-proper-auth-8NRX6

Conversation

@KSXGitHub

@KSXGitHub KSXGitHub commented Apr 29, 2026

Copy link
Copy Markdown
Contributor

Resolves #336.

Summary

Ports pnpm v11's auth flow so pacquet install can talk to private registries on the same footing as pnpm install. Pacquet now parses credentials out of .npmrc, expands ${VAR} references against the process environment, and attaches the right Authorization header on every metadata fetch and tarball download — including tarballs hosted on a CDN host that differs from the registry host.

Upstream reference: pnpm/pnpm@601317e7a3.

What's wired up

  • _auth, _authToken, username, and _password keys, both default-registry (bare) and per-registry (//host[:port]/path/:_authToken=… family), parsed in crates/config (formerly crates/npmrc, renamed by refactor(config): rename pacquet-npmrc → pacquet-config and drop ini wiring #420).
  • ${VAR} and ${VAR:-default} substitution with backslash-escape semantics, ported from @pnpm/config.env-replace. Unresolvable placeholders surface a tracing::warn! and leave the value verbatim — same best-effort behaviour as pnpm's substituteEnv.
  • URL-keyed lookup (AuthHeaders) in crates/network, mirroring createGetAuthHeaderByURI: nerf-darts the request URL, walks parent path prefixes from longest to host-only, falls back to a port-stripped lookup matching upstream's removePort (any port, not just protocol defaults), and prefers inline user:password@ basic auth when present.
  • Default-registry creds key against the resolved registry. The two-phase apply_registry_and_warn / build_auth_headers split inside Config::current ensures pnpm-workspace.yaml's registry override propagates to the bearer-token nerf-dart key.
  • The Authorization header is attached on Package::fetch_from_registry, PackageVersion::fetch_from_registry, and inside DownloadTarballToStore's retry loop.

Test porting checklist

Boxes ticked in plans/TEST_PORTING.md for the upstream auth tests this PR ports:

  • network/auth-header/test/getAuthHeadersFromConfig.test.tsshould convert auth token to Bearer header, should convert basicAuth to Basic header, should handle default registry auth (empty key).
  • network/auth-header/test/getAuthHeaderByURI.ts — all 7 entries (getAuthHeaderByURI(), basic-auth-without-settings, basic-auth-with-settings, https-port-443, default-ports, registry-with-pathnames, default-registry-auth).
  • config/reader/test/parseCreds.test.tsauthToken, authPairBase64, authUsername and authPassword.

The remaining auth checkboxes (tokenHelper, pnpm auth file precedence, redirect header-stripping, the installing/deps-installer/test/install/auth.ts integration tests) need features pacquet doesn't yet have — token helpers, the auth.ini layer, scoped registries, redirect handling — and stay open for follow-ups.

Notes / scope

  • always-auth is intentionally not honoured. pnpm v11 doesn't either — the auth header is selected by URL match alone, and always-auth was deprecated upstream.
  • TLS / proxy / scoped @scope:registry keys remain unparsed; the parser silently ignores them, matching the pre-Support proper auth #336 behaviour. The creds_by_uri shape is ready to grow as those land.
  • The 401/403/404 fail-fast retry policy in crates/tarball was already in place from Tarball fetch has no retry on transient network errors #259; this PR's auth header attaches before the retry loop, so the policy still applies unchanged.
  • Env-var injection follows the trait-per-capability DI pattern from pnpm/pacquet#339: pub trait EnvVar + pub struct RealApi in crates/config/src/api.rs, threaded through env_replace, NpmrcAuth::from_ini, and Config::current under one Api: EnvVar bound. Production callers turbofish RealApi explicitly: Config::current::<RealApi, _, _, _, _>(...).

Test plan

  • cargo nextest run -p pacquet-config -p pacquet-network -p pacquet-tarball -p pacquet-registry --lib (unit tests).
  • cargo clippy --workspace --all-targets -- --deny warnings.
  • cargo fmt --check / taplo format --check / typos.
  • RUSTDOCFLAGS='-D warnings' cargo doc --no-deps --workspace (Doc CI).
  • just registry-mock launch && just test (the registry-mock-bound integration tests can't run in this sandbox; they're independent of the auth changes).

Written by an agent (Claude Code, claude-opus-4-7).

Summary by CodeRabbit

  • New Features

    • Environment-variable substitution in .npmrc (${VAR} and ${VAR:-default}) with pnpm-compatible escapes.
    • Automatic registry authentication: per-registry and default credentials are parsed and applied to metadata and tarball requests.
    • New auth header utilities and richer .npmrc credential parsing/handling.
  • Tests

    • Added extensive unit and integration tests for env substitution, auth/header application, credential parsing (including base64 edge cases), and malformed workspace YAML handling.

Review Change Stack

claude added 2 commits April 29, 2026 17:04
Port pnpm v11's auth flow so `pacquet install` can talk to private
registries on the same footing as `pnpm install`. Parses `_auth`,
`_authToken`, `username`, `_password`, and the per-registry
`//host/path/:_authToken=...` family from `.npmrc`, expands `${VAR}`
references against the process env, and resolves the right
`Authorization` header per request via nerf-darted URI matching --
including default-port stripping and basic-auth in inline URLs.

Headers attach on metadata fetches and tarball downloads alike, so
tarballs hosted on a CDN host that differs from the registry host
still pick up the registry's bearer token. Refs:

* https://github.com/pnpm/pnpm/blob/601317e7a3/network/auth-header/src/index.ts
* https://github.com/pnpm/pnpm/blob/601317e7a3/network/fetch/src/fetchFromRegistry.ts
* https://github.com/pnpm/pnpm/blob/601317e7a3/config/reader/src/getNetworkConfigs.ts
* https://github.com/pnpm/pnpm/blob/601317e7a3/config/reader/src/parseCreds.ts

Resolves #336.
Adopt the trait-per-capability dependency-injection pattern from
[`#332`'s reference comment][1] for environment variable
access used by `${VAR}` substitution in `.npmrc`. Replaces the
`Fn(&str) -> Option<String>` closure parameter on `env_replace`,
`NpmrcAuth::from_ini`, and `Npmrc::current` with a single
`Api: EnvVar` bound; production callers turbofish `RealApi`
explicitly.

Why: keep one DI shape across the codebase as filesystem, disk, and
other process-global capabilities follow. Tests carry their fakes as
zero-sized unit structs (per-test scenario data goes in `static`s),
so coverage of every branch still drops out of pure logic without
mutating the real environment.

Also document the principle in `plans/PORTING_GUIDE.md` so future
ports start from the same template.

[1]: #332 (comment)
@KSXGitHub KSXGitHub requested review from a team, anonrig and zkochan April 29, 2026 17:24
@KSXGitHub

This comment was marked as resolved.

`from_iter` was the original constructor name during draft; the
landed shape exposes `from_creds_map` and `from_map`. The doc
comment was never updated, so `cargo doc --no-deps --workspace`
under `RUSTDOCFLAGS=-D warnings` (the Doc CI job) failed with
`unresolved link to \`AuthHeaders::from_iter\``.
@KSXGitHub KSXGitHub force-pushed the claude/support-proper-auth-8NRX6 branch from 91acdac to f73350b Compare April 29, 2026 17:30
@codecov

codecov Bot commented Apr 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.68404% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.61%. Comparing base (c614d64) to head (ceea639).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
crates/config/src/env_replace.rs 98.46% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #337      +/-   ##
==========================================
+ Coverage   88.64%   89.61%   +0.97%     
==========================================
  Files         116      119       +3     
  Lines        9952    11098    +1146     
==========================================
+ Hits         8822     9946    +1124     
- Misses       1130     1152      +22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented Apr 29, 2026

Copy link
Copy Markdown

Micro-Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.00     15.7±0.12ms   275.6 KB/sec    1.00     15.7±0.09ms   275.8 KB/sec

@github-actions

github-actions Bot commented Apr 29, 2026

Copy link
Copy Markdown

Integrated-Benchmark Report (Linux)

Scenario: Frozen Lockfile

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 2.563 ± 0.091 2.424 2.690 1.00
pacquet@main 2.566 ± 0.081 2.440 2.733 1.00 ± 0.05
pnpm 5.849 ± 0.101 5.713 6.046 2.28 ± 0.09
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 2.56295427374,
      "stddev": 0.09143204579674404,
      "median": 2.56881930374,
      "user": 2.63285444,
      "system": 3.5326441799999997,
      "min": 2.42353422024,
      "max": 2.68964492624,
      "times": [
        2.6859229632400003,
        2.4863806422400003,
        2.55195909524,
        2.47105080524,
        2.59901133524,
        2.5036552802400003,
        2.63270395724,
        2.68964492624,
        2.58567951224,
        2.42353422024
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 2.56560512364,
      "stddev": 0.0807797201589505,
      "median": 2.55937061924,
      "user": 2.6330177400000006,
      "system": 3.5400617800000007,
      "min": 2.44007517524,
      "max": 2.73291688624,
      "times": [
        2.52309530024,
        2.57018271424,
        2.61240430324,
        2.50465201524,
        2.73291688624,
        2.58715955024,
        2.44007517524,
        2.62538840424,
        2.51161836324,
        2.54855852424
      ]
    },
    {
      "command": "pnpm",
      "mean": 5.84895801804,
      "stddev": 0.10051865395003538,
      "median": 5.8489184517399995,
      "user": 8.443849539999999,
      "system": 4.33443578,
      "min": 5.71316713524,
      "max": 6.0462559032400005,
      "times": [
        5.83925825524,
        5.85857864824,
        5.79498358124,
        5.74624714524,
        5.8731836822400005,
        5.9285640312400005,
        5.71316713524,
        6.0462559032400005,
        5.92517001324,
        5.76417178524
      ]
    }
  ]
}

Scenario: Frozen Lockfile (Hot Cache)

Command Mean [ms] Min [ms] Max [ms] Relative
pacquet@HEAD 743.3 ± 62.7 696.3 908.2 1.00
pacquet@main 774.5 ± 72.3 713.6 915.7 1.04 ± 0.13
pnpm 2447.6 ± 122.4 2321.5 2640.5 3.29 ± 0.32
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 0.74333362206,
      "stddev": 0.06268485532146142,
      "median": 0.72464912896,
      "user": 0.36694229999999994,
      "system": 1.5965957999999998,
      "min": 0.69630385546,
      "max": 0.90823749746,
      "times": [
        0.90823749746,
        0.76968127146,
        0.74570640946,
        0.73621455246,
        0.69630385546,
        0.71308370546,
        0.70421685246,
        0.74717220146,
        0.70908961646,
        0.70363025846
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 0.77454669316,
      "stddev": 0.07225391661825549,
      "median": 0.73771252696,
      "user": 0.37953669999999995,
      "system": 1.6233929,
      "min": 0.71358953646,
      "max": 0.91566718646,
      "times": [
        0.73458929046,
        0.71436835246,
        0.71358953646,
        0.87616403346,
        0.81607686046,
        0.78355280946,
        0.72212669546,
        0.91566718646,
        0.74083576346,
        0.72849640346
      ]
    },
    {
      "command": "pnpm",
      "mean": 2.44756680836,
      "stddev": 0.12244989237597423,
      "median": 2.39816370596,
      "user": 2.8571038999999994,
      "system": 2.1793402,
      "min": 2.32150295446,
      "max": 2.6404661654600003,
      "times": [
        2.47309205146,
        2.33938854746,
        2.4137066494600004,
        2.6404661654600003,
        2.3676568984600004,
        2.38262076246,
        2.32150295446,
        2.34424779346,
        2.6332792914600005,
        2.55970696946
      ]
    }
  ]
}

claude added 4 commits April 29, 2026 18:32
The codecov bot flagged 32 lines on the auth PR. After investigating
each:

* `crates/network/src/auth.rs`: now 100%. New tests cover the
  trailing-slash-append branch, malformed-URL fallthrough, no-path
  authority, and the empty-`@host` short-circuit.
* `crates/npmrc/src/npmrc_auth.rs`: now 100%. New tests cover the
  `[section]` skip, env-replace failure on the key, top-level
  `_auth` / `username` / `_password`, lone per-registry password,
  per-registry creds through `build_auth_headers`, unknown
  per-registry suffix, warning drain via `apply_registry_and_warn`,
  invalid-base64 password fallback, and every base64 alphabet arm.
  Also unifies `apply_creds_field` and `apply_default_creds_field`
  so the catch-all is exercised by `ignores_non_auth_keys`.
* `crates/npmrc/src/env_replace.rs`: now 100% executable. New test
  covers the trailing-`$` early-`None` branch in
  `find_placeholder_end`. The two test fakes that previously used
  `unreachable!()` to assert "this lookup must not run" now use
  `NoEnv` directly — the assertion was test-internal documentation
  and the parser short-circuit is what the test actually verifies.
* `crates/registry/src/package.rs` and
  `crates/registry/src/package_version.rs`: new `mockito` tests
  prove the `Authorization` header reaches the server on metadata
  GETs (both `/<pkg>` and `/<pkg>/<tag>`).
* `crates/tarball/src/tests.rs`: new test proves the
  `Authorization` header reaches the server on tarball downloads.

The remaining 3 uncovered lines live in `crates/npmrc/src/api.rs`
(`RealApi::var` delegating to `std::env::var`). They are the bottom
of the dependency-injection stack and are excluded from unit
coverage by design — see [the DI principle][1] from
`#339`. Mutating the real process environment to test
the production glue would (a) be flaky in parallel test runs and
(b) not exercise any pacquet logic.

[1]: #339
Main now points at `plans/TEST_PORTING.md` as the canonical
test-porting plan and adds a "Support Proper Auth" section
enumerating the upstream tests this PR's issue (#336) should port.
That section, along with the DI guide planned for `CODE_STYLE_GUIDE.md`
in #339, supersedes the standalone `plans/PORTING_GUIDE.md`
this branch had introduced — drop it and rewire every doc link to
point at the issue instead.

Tick the auth checkboxes in `TEST_PORTING.md` for the upstream tests
this PR ports, leaving `tokenHelper`, `auth.ini` precedence, and
the integration-level fetcher tests for follow-ups.

[1]: #339

Copy link
Copy Markdown
Contributor Author

@zkochan — heads up that the test-the-tests practice from plans/TEST_PORTING.md (temporarily break the subject-under-test, run the test, confirm it fails for the right reason) was not applied to the new tests this PR introduces.

The reason is sandbox-side: just ready aborts on pacquet-registry-mock launch because portpicker::pick_unused_port() returns None in this sandbox, and the same crash kills every pacquet-cli integration test that tries to start a mock registry. The mockito-based unit/integration tests below run fine in isolation (cargo nextest run -p <crate> --lib), so I confirmed they pass — but I never confirmed they fail under a regression in the subject.

If you have a local checkout with a working registry-mock, would you mind running the break-then-verify pass on these? Suggested break sites are minimal one-line edits, and your Claude Code agent should be able to drive them autonomously.

Test Break the subject by
pacquet-network::auth::tests::matches_path_scoped_token In crates/network/src/auth.rs, change the loop bound for i in (3..=upper).rev() inside lookup_by_nerf to for i in (4..=upper).rev() — should drop the host-only fallback and break the host-only assertion at the end of matches_host_only_token, plus the path-scoped expectation.
pacquet-network::auth::tests::default_https_port_strips_for_lookup In crates/network/src/auth.rs::AuthHeaders::for_url, delete the if parsed.has_default_port() { … } retry block.
pacquet-network::auth::tests::host_only_url_without_trailing_slash_still_matches In crates/network/src/auth.rs::AuthHeaders::for_url, replace the trailing-slash-append branch with let url_with_slash = url;.
pacquet-network::auth::tests::nerf_dart_handles_url_with_no_path_separator In crates/network/src/auth.rs::ParsedUrl::parse, change None => (rest, "") to None => return None.
pacquet-npmrc::npmrc_auth::tests::parses_default_auth_token_and_keys_to_registry In crates/npmrc/src/npmrc_auth.rs::build_auth_headers, replace nerf_dart(&npmrc.registry) with npmrc.registry.clone().
pacquet-npmrc::npmrc_auth::tests::per_registry_username_password_apply_through_build_auth_headers In crates/npmrc/src/npmrc_auth.rs::creds_to_header, delete the if let (Some(user), Some(pass_b64)) = … arm.
pacquet-npmrc::npmrc_auth::tests::env_replace_substitutes_token In crates/npmrc/src/npmrc_auth.rs::from_ini, replace env_replace::<Api>(raw_value) with Ok::<_, env_replace::EnvReplaceError>(raw_value.to_owned()) so substitution is bypassed.
pacquet-npmrc::env_replace::tests::uses_default_when_variable_unset In crates/npmrc/src/env_replace.rs::env_replace, change the (None, Some(default)) arm to (None, _) returning Err.
pacquet-registry::package::tests::fetch_from_registry_attaches_authorization_header In crates/registry/src/package.rs::Package::fetch_from_registry, delete the if let Some(value) = auth_headers.for_url(&url()) { … } block.
pacquet-registry::package_version::tests::fetch_from_registry_attaches_authorization_header Same as above in crates/registry/src/package_version.rs::PackageVersion::fetch_from_registry.
pacquet-tarball::tests::fetch_attaches_authorization_header_when_creds_match_tarball_url In crates/tarball/src/lib.rs::fetch_and_extract_once, delete the if let Some(value) = auth_headers.for_url(package_url) { … } block.

Each break should make exactly the listed test fail. If any of them stay green after the break, that's a real coverage hole and I'll fix it before this lands.


Generated by Claude Code

claude added 2 commits April 30, 2026 03:04
Per `plans/TEST_PORTING.md`'s "test the tests" practice, every new
test in this PR was verified by temporarily breaking its
subject-under-test and confirming the test fails for the right
reason. Two flaws surfaced:

* `host_only_url_without_trailing_slash_still_matches` did not
  actually depend on the trailing-slash-append branch in
  `AuthHeaders::for_url`. Removing the branch left the test green
  because `ParsedUrl::parse`'s no-path-separator handling gave the
  same `//reg.com/` nerf-dart for both `https://reg.com` and
  `https://reg.com/`. Replace it with
  `slash_append_branch_lets_path_segment_match`: the new test uses
  `https://reg.com/scope` against a `//reg.com/scope/` token, so
  removing the slash-append makes `nerf_dart` drop the `scope`
  segment and the lookup correctly fails.

* The explicit `[section]` skip in `NpmrcAuth::from_ini` was dead
  code — the no-`=` branch one line below already handled section
  headers. Drop the skip; rename the test to reflect what it
  actually verifies.

Every other new test now has at least one minimal break that makes
it (and only it, where applicable) fail. The same matrix is in the
PR comment for @zkochan to re-run locally if desired.
Codecov's latest patch report flagged `crates/npmrc/src/lib.rs` at
89.18% with 4 missing lines, three of which sat in the
`NoEnv::var` body of the lib.rs test module. The body was
unreachable from `lib.rs` tests because every `.npmrc` fixture
there is placeholder-free, so `env_replace` short-circuits before
calling `Api::var`.

Add a new test that feeds `${MISSING_TOKEN}` through the parser via
`Npmrc::current::<NoEnv, ...>(...)`, asserts the warning path leaves
the literal placeholder verbatim, and incidentally drives the
generic `Api: EnvVar` bound end-to-end.

The remaining uncovered lines on this PR's diff are all
intentionally exempt: `RealApi::var` is the production
`std::env::var` bridge (excluded by #339's "When DI is
not needed" rule), and four `|| unreachable!("shouldn't reach home
dir")` closures encode test invariants that are unreachable by
design.

This comment was marked as resolved.

KSXGitHub

This comment was marked as resolved.

Comment thread crates/cli/tests/install.rs Outdated
@KSXGitHub KSXGitHub force-pushed the claude/support-proper-auth-8NRX6 branch from 0e5a3e4 to fd2d44a Compare April 30, 2026 16:32
`<RealApi as EnvVar>::var` (the `std::env::var` bridge in
`crates/npmrc/src/api.rs`) is unreachable by every other test
because `add_mocked_registry` writes literal `.npmrc` values. The
parser's no-`$` short-circuit fires before the env-var pipeline is
reached.

Add an integration test that rewrites the registry URL to
`${PACQUET_TEST_REGISTRY}`, sets the variable on the spawned
binary's environment via `command_extra::CommandExtra::with_env`,
and asserts the install still succeeds. The path then runs through
`Npmrc::current::<RealApi, _, _, _, _>`, `NpmrcAuth::from_ini`,
`env_replace::<RealApi>`, and `RealApi::var`, closing the codecov
hole on `crates/npmrc/src/api.rs`.

Mirrors pnpm's [`installing/deps-installer/test/install/auth.ts`](https://github.com/pnpm/pnpm/blob/601317e7a3/installing/deps-installer/test/install/auth.ts)
in shape.
@KSXGitHub KSXGitHub force-pushed the claude/support-proper-auth-8NRX6 branch from fd2d44a to b15f887 Compare April 30, 2026 17:54
claude added 2 commits April 30, 2026 18:39
Follow the new style rules from #352 (use rustdoc intra-doc links
for in-scope identifiers in doc comments) across the auth-related
files added by this PR.

Notable fixes:
* `crates/npmrc/src/env_replace.rs` had a non-resolving
  `[\`EnvReplaceError::Missing\`]` reference (`EnvReplaceError` is a
  struct, not an enum). Drop the bogus `::Missing` segment and link
  the type itself.
* Test docstrings across `auth.rs`, `npmrc_auth.rs`, and `lib.rs`
  now use intra-doc links for the in-scope items they reference
  (`nerf_dart`, `ParsedUrl::parse`, `creds_to_header`,
  `split_creds_key`, `apply_creds_field`, `NpmrcAuth::*`,
  `Npmrc::current`, `EnvVar::var`, `env_replace`, etc.).
* Registry test docstrings link `Package::fetch_from_registry` and
  `PackageVersion::fetch_from_registry` rather than naming them in
  bare backticks.

PR 351 ("don't reference private items from public docs") audited
in the same pass; the new pub items in this PR only reference other
pub items in their docs, so no changes were needed for that rule.
The integration test in `crates/cli/tests/install.rs` keeps bare
backticks because rustdoc does not process integration-test files
and the imported scope there does not include the referenced
`pacquet_npmrc` items.
claude added 2 commits April 30, 2026 20:40
# Conflicts:
#	crates/npmrc/src/npmrc_auth.rs
claude added 5 commits May 13, 2026 16:05
Pull in 4 commits from upstream main:
- feat(lockfile): BinaryResolution + VariationsResolution (#457)
- feat: hoisting support (hoistPattern + publicHoistPattern) (#445)
- test(git-fetcher): port §E git-fetcher tests (#462)
- feat(real-hoist): ancestor-path-aware peer-shadow refusal (#461)

Conflict: `crates/config/src/lib.rs` — the hoisting PR (#445)
added `pub mod matcher;` adjacent to my `mod env_replace;`. Keep
both module declarations.
Lines that became too long after the `Npmrc` → `Config` /
`#[allow]` → `#[expect]` swaps got reflowed by `cargo fmt`.
Caught by the Format CI check.
Upstream `getAuthHeaderByURI`'s second pass calls
[`removePort`](https://github.com/pnpm/pnpm/blob/601317e7a3/network/auth-header/src/helpers/removePort.ts)
unconditionally and retries iff the URL changed (port was set).
Pacquet only stripped 443/80, so a `.npmrc` keyed `//reg.com/`
missed a request to `https://reg.com:8080/` even though pnpm
matches it. Strip any port on the fallback pass to close the
divergence.

Also:
- pub(crate) on `NpmrcAuth`, `RawCreds`, `EnvReplaceError`,
  `env_replace`. They have no external consumers and the public
  modifier was misleading inside private modules.
- Restructure four em-dash mid-sentences in new doc comments
  per CONTRIBUTING.md.
- Drop the dead `scheme` field on `ParsedUrl` (only consumer
  was the just-removed `has_default_port`).
Two cardinal-rule divergences caught by a second-round review:

1. `nerf_dart` kept default ports verbatim. Upstream uses
   WHATWG `URL.host` ([@pnpm/config.nerf-dart](https://github.com/pnpm/components/blob/a8ba7794d8/config/nerf-dart/nerf-dart.ts))
   which drops `:443`/`:80`. A registry configured as
   `https://reg.com:443/` keyed creds at `//reg.com:443/`, then
   a request to `https://reg.com/...` (no explicit port) missed,
   because the port-strip fallback only fires when the *request*
   URL carries a port. Strip default ports inside `ParsedUrl::nerf_dart`
   so both the keying and the lookup paths agree with upstream.

2. `env_replace` counted backslashes from the working `output`
   buffer rather than from the source. A previously-substituted
   variable whose value ended in `\` would conflate with literal
   source `\` characters preceding the next `${...}`. Upstream's
   `(?<!\\)(\\*)\$\{...}` runs on the original input, so a single
   literal `\` between two placeholders must escape only the
   second one regardless of any trailing `\` in the first's value.
   Walk `bytes` (the source) backward from `index-1` instead.

Both come with regression tests (`nerf_dart_strips_default_ports_when_keying`,
`backslash_count_uses_source_not_output_buffer`) verified by
break-then-run.
Round 3 review caught two issues:

1. The `auth.rs` module-level doc claimed cross-host CDN tarballs
   would still pick up registry creds via "fall through to the
   host-only key for that registry." That misdescribes what
   `lookup_by_nerf` does: it walks parts of the *request* URL, so a
   different host can never reach a different host's keys. Pacquet
   matches pnpm here. Reword the doc to spell out the actual
   semantics: cross-host means no header, place a key at the CDN's
   host or a path prefix to attach one.

2. `fetch_attaches_authorization_header_when_creds_match_tarball_url`
   only mocks `expect(1)`, so a regression that read `auth_headers`
   once outside the retry loop would slip through. Add
   `retry_re_attaches_authorization_header_on_each_attempt`: mock
   503 then 200, both gated on `match_header("authorization", ...)`.
   Verified via break-then-run that swapping the second attempt's
   `auth_headers` for `&AuthHeaders::default()` causes the test to
   fail.
@KSXGitHub KSXGitHub marked this pull request as ready for review May 13, 2026 17:39
Copilot AI review requested due to automatic review settings May 13, 2026 17:39
@KSXGitHub KSXGitHub marked this pull request as draft May 13, 2026 17:44

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/cli/tests/install.rs`:
- Around line 281-282: Add diagnostic output before the non-assert_eq!
assertions (e.g., the assert_ne!(original, patched, ...) and the other assertion
block around lines 300-303) so CI failures show concrete values; print the
relevant variables and paths (original, patched, &npmrc_path) using eprintln! or
println! just before the assert_ne! and before the fs::write/asserts so test
logs include the actual contents and the .npmrc path for easier triage.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 394d15f4-f892-4365-8c2b-fa034edef44c

📥 Commits

Reviewing files that changed from the base of the PR and between c614d64 and df919b2.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (24)
  • crates/cli/src/cli_args.rs
  • crates/cli/tests/install.rs
  • crates/config/Cargo.toml
  • crates/config/src/api.rs
  • crates/config/src/env_replace.rs
  • crates/config/src/lib.rs
  • crates/config/src/npmrc_auth.rs
  • crates/config/src/npmrc_auth/tests.rs
  • crates/config/src/workspace_yaml/tests.rs
  • crates/network/Cargo.toml
  • crates/network/src/auth.rs
  • crates/network/src/lib.rs
  • crates/package-manager/src/add.rs
  • crates/package-manager/src/install_package_by_snapshot.rs
  • crates/package-manager/src/install_package_from_registry.rs
  • crates/package-manager/src/install_package_from_registry/tests.rs
  • crates/registry/Cargo.toml
  • crates/registry/src/package.rs
  • crates/registry/src/package/tests.rs
  • crates/registry/src/package_version.rs
  • crates/tarball/src/lib.rs
  • crates/tarball/src/tests.rs
  • plans/TEST_PORTING.md
  • tasks/micro-benchmark/src/main.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Agent
  • GitHub Check: Run benchmark on ubuntu-latest
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Preserve existing method chains and pipe-trait chains; do not break them into intermediate let bindings unless there is a concrete justification such as a compilation failure, borrow checker rejection, meaningful performance improvement, or other technical necessity. Refactoring for style alone is not sufficient justification.
Choose owned vs. borrowed parameters to minimize copies; prefer borrowed types (&Path over &PathBuf, &str over &String) when it does not force extra copies.
Prefer Arc::clone(&x) and Rc::clone(&x) over x.clone() for reference-counted types to make the cost visible at the call site.
Do not use star imports inside module bodies. Write use super::{Foo, bar} instead of use super::*; for any glob whose target is a module you control. External-crate preludes (e.g., use rayon::prelude::*;) and root-of-module re-exports (e.g., pub use submodule::*; in lib.rs) are exceptions.
Follow Rust API Guidelines for naming, as documented in https://rust-lang.github.io/api-guidelines/naming.html.
Declare a newtype wrapper for any branded string type being ported from TypeScript pnpm. Do not collapse the brand into a plain String or &str; give the type its own struct so misuse is a type error.
When porting branded string types where upstream TypeScript always validates before construction, validate in the Rust port too. Construct the wrapper only via TryFrom<String> and/or FromStr; do not provide an infallible public constructor that takes an arbitrary string.
For branded string types where upstream TypeScript never validates (used purely for type-safety to prevent confusion between string slots), expose an infallible From<String> and From<&str> constructor in the Rust wrapper.
When upstream TypeScript occasionally constructs a branded type without validation (via bare as assertion), add a from_str_unchecked (or similarly named) constructor on the Rust side. Keep the validating constructor as well; `from_str_u...

Files:

  • crates/package-manager/src/add.rs
  • crates/package-manager/src/install_package_from_registry/tests.rs
  • tasks/micro-benchmark/src/main.rs
  • crates/package-manager/src/install_package_by_snapshot.rs
  • crates/registry/src/package/tests.rs
  • crates/config/src/workspace_yaml/tests.rs
  • crates/cli/tests/install.rs
  • crates/registry/src/package.rs
  • crates/network/src/lib.rs
  • crates/package-manager/src/install_package_from_registry.rs
  • crates/registry/src/package_version.rs
  • crates/cli/src/cli_args.rs
  • crates/config/src/api.rs
  • crates/network/src/auth.rs
  • crates/config/src/npmrc_auth/tests.rs
  • crates/config/src/env_replace.rs
  • crates/tarball/src/lib.rs
  • crates/config/src/npmrc_auth.rs
  • crates/config/src/lib.rs
  • crates/tarball/src/tests.rs
**/tests/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/tests/**/*.rs: When porting behavior from pnpm, port the relevant pnpm tests to Rust tests whenever they translate. Matching test coverage is the easiest way to prove behavioral parity.
Consult the test-porting plan in plans/TEST_PORTING.md before adding ported tests. Follow the conventions expected for ports: use known_failures modules, use pacquet_testing_utils::allow_known_failure! at the not-yet-implemented boundary, and temporarily break the subject under test to verify the ported test actually catches the regression. Update TEST_PORTING.md checkboxes as items land.
Follow the test-logging guidance in CODE_STYLE_GUIDE.md: log before non-assert_eq! assertions, use dbg! for complex structures, skip logging for simple scalar assert_eq! assertions.

Files:

  • crates/cli/tests/install.rs
**/*.md

📄 CodeRabbit inference engine (AGENTS.md)

When citing upstream pnpm code anywhere (code comments, doc comments, Markdown docs, PR descriptions, or commit messages), link to a specific commit SHA, not a branch name. Use the first 10 hex characters of the SHA. Branch links like github.com/<owner>/<repo>/blob/main/... are impermanent; permanent links pin the commit so the reference stays meaningful long after upstream changes. This rule applies to references to any GitHub repository, not only pnpm/pnpm.

Files:

  • plans/TEST_PORTING.md
🧠 Learnings (4)
📚 Learning: 2026-05-07T23:19:08.272Z
Learnt from: KSXGitHub
Repo: pnpm/pacquet PR: 401
File: tasks/integrated-benchmark/src/work_env.rs:343-344
Timestamp: 2026-05-07T23:19:08.272Z
Learning: When reviewing Rust code in pnpm/pacquet for deprecated API usage, do not automatically treat `serde_saphyr::to_string` as deprecated. In `serde-saphyr` v0.0.25, `serde_saphyr::to_string` has no `#[deprecated]` attribute (the `#[deprecated]` later in `serde-saphyr-0.0.25/src/lib.rs` applies to a different function). Only flag `serde_saphyr::to_string` as deprecated if the resolved dependency version’s source shows `#[deprecated]` on that specific function.

Applied to files:

  • crates/package-manager/src/add.rs
  • crates/package-manager/src/install_package_from_registry/tests.rs
  • tasks/micro-benchmark/src/main.rs
  • crates/package-manager/src/install_package_by_snapshot.rs
  • crates/registry/src/package/tests.rs
  • crates/config/src/workspace_yaml/tests.rs
  • crates/cli/tests/install.rs
  • crates/registry/src/package.rs
  • crates/network/src/lib.rs
  • crates/package-manager/src/install_package_from_registry.rs
  • crates/registry/src/package_version.rs
  • crates/cli/src/cli_args.rs
  • crates/config/src/api.rs
  • crates/network/src/auth.rs
  • crates/config/src/npmrc_auth/tests.rs
  • crates/config/src/env_replace.rs
  • crates/tarball/src/lib.rs
  • crates/config/src/npmrc_auth.rs
  • crates/config/src/lib.rs
  • crates/tarball/src/tests.rs
📚 Learning: 2026-05-01T10:01:33.766Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 349
File: crates/reporter/src/tests.rs:121-121
Timestamp: 2026-05-01T10:01:33.766Z
Learning: In Rust test code, follow the repo’s CODE_STYLE_GUIDE test-logging rule: add logging (e.g., `eprintln!`/`eprintln!(...)`) so that useful diagnostic values are printed when a test fails, unless the assertion is `assert_eq!` (where the differing values are already included). Concretely, if you use assertions like `assert!`, `assert_ne!`, etc., ensure the test logs the relevant actual/expected values (or context) before/around the assertion so failures can be diagnosed without rerunning.

Applied to files:

  • crates/package-manager/src/install_package_from_registry/tests.rs
  • crates/registry/src/package/tests.rs
  • crates/config/src/workspace_yaml/tests.rs
  • crates/config/src/npmrc_auth/tests.rs
  • crates/tarball/src/tests.rs
📚 Learning: 2026-05-07T14:24:47.105Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 391
File: crates/cli/tests/lifecycle_scripts.rs:0-0
Timestamp: 2026-05-07T14:24:47.105Z
Learning: In pnpm/pacquet CLI lifecycle tests, note that `AutoMockInstance::load_or_init` returns an anchor to a shared singleton mock registry process. If a test spawns a secondary `CommandTempCwd::init().add_mocked_registry()` (e.g., to run a reinstall with `--frozen-lockfile`), the secondary/inner `mock_instance` may be dropped safely as long as the primary/outer `mock_instance` remains in scope (so the singleton registry stays alive). Separately retain the inner `TempDir` (e.g., via a `frozen_root` binding) so the workspace lives for the duration of the command.

Applied to files:

  • crates/cli/tests/install.rs
📚 Learning: 2026-05-01T10:01:33.766Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 349
File: crates/reporter/src/tests.rs:121-121
Timestamp: 2026-05-01T10:01:33.766Z
Learning: In this repo’s Rust tests (run via `cargo nextest`), stdout/stderr from passing tests is captured and is only shown when a test fails. As a result, `eprintln!` (and similar stderr logging) in these tests should generally not be flagged as CI “noise” on the happy path; CI output should only appear for failing tests.

Applied to files:

  • crates/tarball/src/tests.rs
🔇 Additional comments (26)
crates/config/src/env_replace.rs (1)

102-106: Verify bare-dash form and empty-string semantics match pnpm.

The current implementation only recognizes ${VAR:-default} (colon-dash) and unconditionally filters empty variable values before checking for defaults. Upstream pnpm's regex accepts both ${VAR:-default} and ${VAR-default} with different empty-string behavior: bare-dash preserves empty strings, colon-dash replaces them with the default.

Run the following to confirm upstream behavior:

#!/bin/bash
# Verify pnpm's env-replace supports both :- and - forms
cd /tmp
git clone --depth 1 https://github.com/pnpm/components.git pnpm-components 2>/dev/null || true
cd pnpm-components
cat config/env-replace/env-replace.ts | grep -A5 'const regex'
crates/network/src/auth.rs (3)

67-67: 💤 Low value

Consider reducing visibility to pub(crate).

from_map is only called once within this crate (by from_creds_map). Per the project's public API minimality principle, it could be pub(crate) to avoid expanding the public surface unnecessarily.


125-125: 💤 Low value

Loop range has an off-by-one relative to upstream.

Upstream's getAuthHeaderByURI loop is for (let i = Math.min(parts.length, maxParts) - 1; i >= 3; i--). The Rust port uses (3..=upper).rev() which includes one extra iteration at the high end. While functionally benign (the extra lookup never matches), it diverges from the spec.

♻️ Suggested fix
-        for i in (3..=upper).rev() {
+        // Match upstream's `Math.min(...) - 1` high bound
+        for i in (3..upper).rev() {

183-188: 💤 Low value

Clarify IPv6 comment.

The comment says "Skip IPv6 brackets" but the code doesn't skip them—it treats [::1]:8080 as the entire host (with port = None). What's actually skipped is the default-port stripping pass for IPv6 addresses.

♻️ Suggested rewording
-            // Skip IPv6 brackets. Pnpm doesn't handle them either, and
-            // no npm registry we care about uses them. Documenting the
-            // limit here rather than silently misparsing.
+            // Treat `[…]:port` as the entire host; `nerf_dart` produces
+            // the correct key but `is_default_port` doesn't fire for IPv6
+            // (so explicit default ports won't be stripped). Pnpm doesn't
+            // handle IPv6 either, and no npm registry uses it in practice.
crates/registry/Cargo.toml (1)

28-28: LGTM!

crates/network/Cargo.toml (1)

17-19: LGTM!

crates/config/src/workspace_yaml/tests.rs (1)

432-451: LGTM!

plans/TEST_PORTING.md (1)

443-465: LGTM!

crates/config/src/api.rs (1)

1-52: LGTM!

crates/network/src/lib.rs (1)

1-3: LGTM!

crates/config/Cargo.toml (1)

14-14: LGTM!

Also applies to: 27-27

crates/package-manager/src/install_package_by_snapshot.rs (1)

159-159: LGTM!

crates/package-manager/src/install_package_from_registry/tests.rs (1)

56-56: LGTM!

tasks/micro-benchmark/src/main.rs (1)

6-6: LGTM!

Also applies to: 53-53

crates/package-manager/src/add.rs (1)

71-71: LGTM!

crates/cli/src/cli_args.rs (1)

12-12: LGTM!

Also applies to: 96-103

crates/package-manager/src/install_package_from_registry.rs (1)

88-100: LGTM!

Also applies to: 164-164

crates/config/src/npmrc_auth.rs (2)

187-196: Duplicate: invalid _password decode fallback still diverges from pnpm auth parsing.

Line 195 still falls back to raw _password when base64 decoding fails, which preserves a behavior already flagged as diverging from upstream parity expectations.

As per coding guidelines: "If pnpm's main branch and this repo disagree on behavior, pnpm's main is the source of truth. Reconcile toward upstream, not away from it."


1-186: LGTM!

Also applies to: 197-265

crates/registry/src/package/tests.rs (1)

6-7: LGTM!

Also applies to: 47-79

crates/registry/src/package.rs (1)

6-7: LGTM!

Also applies to: 34-49

crates/tarball/src/lib.rs (1)

13-13: LGTM!

Also applies to: 930-937, 1051-1054, 1063-1064, 1073-1081, 1105-1105, 1311-1312, 1323-1324, 1507-1508, 1590-1591

crates/tarball/src/tests.rs (1)

6-6: LGTM!

Also applies to: 178-179, 226-227, 301-302, 360-361, 587-588, 646-647, 708-709, 780-781, 998-999, 1045-1046, 1076-1077, 1116-1117, 1150-1151, 1257-1259, 1273-1274, 1341-1342, 1349-1450, 1514-1515, 1540-1541, 1612-1613, 1628-1629, 1728-1729, 1817-1818, 1906-1907, 1942-1943, 2019-2020

crates/registry/src/package_version.rs (1)

3-3: LGTM!

Also applies to: 32-47, 87-135

crates/config/src/lib.rs (1)

1-1: LGTM!

Also applies to: 3-3, 10-11, 473-479, 611-617, 643-657, 741-746, 782-783, 853-854, 871-876, 895-900, 912-918, 927-928, 946-947, 964-970, 977-978, 998-1004, 1023-1029, 1048-1054, 1077-1083, 1103-1107, 1132-1138, 1172-1178, 1216-1217, 1255-1261

crates/config/src/npmrc_auth/tests.rs (1)

1-312: LGTM!

Comment thread crates/cli/tests/install.rs
Round 4 review:
- `build_auth_headers` now passes default-registry creds with an
  empty-string key into `AuthHeaders::from_creds_map`, matching
  upstream's
  [`getAuthHeadersFromCreds`](https://github.com/pnpm/pnpm/blob/601317e7a3/network/auth-header/src/getAuthHeadersFromConfig.ts)
  convention (`configByUri['']` holds default creds and the
  constructor re-keys them). The `from_creds_map` constructor's
  empty-key path is now live in production instead of test-only.
- Drop the stale `nerf_dart` import from `npmrc_auth.rs`.
- Reword the `slash_append_branch_lets_path_segment_match` test
  doc: the `registry_with_pathname_matches_metadata_and_tarballs`
  test's first assertion (URL without trailing slash) does also
  exercise the slash-append branch. Keep the focused single-
  assertion test, but stop claiming it's the sole guard.

CodeRabbit inline comment on `crates/cli/tests/install.rs:282`:
- Log concrete values before the `assert_ne!`/`assert!` calls per
  CODE_STYLE_GUIDE.md's "log before non-`assert_eq!` assertions"
  rule. CI failures now print `original_npmrc`, `patched_npmrc`,
  `npmrc_path`, and `symlink_path`.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Ports pnpm v11-style authentication into pacquet so installs can access private registries/CDN tarballs by parsing .npmrc credentials (with ${VAR} expansion) and attaching the correct Authorization header on registry metadata and tarball downloads.

Changes:

  • Added .npmrc auth parsing in pacquet-config (default + per-registry creds) with ${VAR} / ${VAR:-default} substitution via DI (EnvVar/RealApi).
  • Introduced pacquet-network::AuthHeaders URL-keyed lookup (nerf-dart + longest-prefix matching + basic-auth-in-URL precedence).
  • Threaded AuthHeaders through registry metadata fetches and tarball download/retry paths; added unit + integration tests and updated porting checklist.

Reviewed changes

Copilot reviewed 24 out of 25 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
tasks/micro-benchmark/src/main.rs Updates benchmark tarball downloader construction to supply AuthHeaders.
plans/TEST_PORTING.md Marks upstream auth tests as ported/covered.
crates/tarball/src/tests.rs Threads AuthHeaders through tarball tests; adds assertions that auth headers are attached per attempt.
crates/tarball/src/lib.rs Adds auth_headers to tarball download API and attaches Authorization per request (inside retry flow).
crates/registry/src/package/tests.rs Adds mock-based test ensuring registry metadata requests include Authorization.
crates/registry/src/package.rs Accepts AuthHeaders and attaches Authorization for package metadata fetches.
crates/registry/src/package_version.rs Accepts AuthHeaders and attaches Authorization for tag/version metadata fetches; adds test.
crates/registry/Cargo.toml Adds mockito dev-dependency for new tests.
crates/package-manager/src/install_package_from_registry/tests.rs Initializes Config with auth_headers in test helper.
crates/package-manager/src/install_package_from_registry.rs Passes auth_headers through registry fetch + tarball download.
crates/package-manager/src/install_package_by_snapshot.rs Passes auth_headers into tarball download path.
crates/package-manager/src/add.rs Passes auth_headers when resolving latest package version.
crates/network/src/lib.rs Exposes new auth module and re-exports AuthHeaders/helpers.
crates/network/src/auth.rs Implements nerf-dart + auth-header lookup + base64 helper with tests.
crates/network/Cargo.toml Adds pretty_assertions for auth module tests.
crates/config/src/workspace_yaml/tests.rs Adds test asserting malformed YAML surfaces as ParseYaml.
crates/config/src/npmrc_auth/tests.rs Expands coverage for auth parsing, env substitution warnings, and base64 decoding behavior.
crates/config/src/npmrc_auth.rs Implements two-phase .npmrc application and builds AuthHeaders from parsed creds.
crates/config/src/lib.rs Adds auth_headers to Config; makes Config::current generic over Api: EnvVar and performs two-phase .npmrc apply.
crates/config/src/env_replace.rs Adds pnpm-compatible env placeholder substitution implementation + tests.
crates/config/src/api.rs Introduces EnvVar capability and RealApi provider (std env bridge).
crates/config/Cargo.toml Adds deps needed for auth/env-substitution (pacquet-network, tracing).
crates/cli/tests/install.rs Adds end-to-end test verifying ${VAR} substitution works in spawned CLI process.
crates/cli/src/cli_args.rs Calls Config::current with explicit RealApi turbofish.
Cargo.lock Updates lockfile for new intra-workspace dependencies/dev-deps.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/cli/tests/install.rs Outdated
/// End-to-end coverage for `${VAR}` substitution in `.npmrc`.
///
/// `<RealApi as EnvVar>::var` (the `std::env::var` bridge in
/// `crates/npmrc/src/api.rs`) is unreachable by every other test

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied in 947bf6a.


Written by an agent (Claude Code, claude-opus-4-7).


Generated by Claude Code

Comment thread crates/network/src/auth.rs Outdated
Comment on lines +41 to +55
/// Build an [`AuthHeaders`] from `(nerf_darted_uri, header_value)`
/// pairs. Caller is responsible for nerf-darting and for choosing
/// the right scheme (`Bearer ...` or `Basic ...`).
///
/// The `default_registry_uri` argument is the URI to register
/// against the `default` (empty-string) credentials slot, matching
/// `createGetAuthHeaderByURI`'s `defaultRegistry` argument and
/// applying the same fallback to `//registry.npmjs.org/` when
/// nothing is supplied.
pub fn from_creds_map<Iter>(headers: Iter, default_registry_uri: Option<&str>) -> Self
where
Iter: IntoIterator<Item = (String, String)>,
{
let registry_default_key =
default_registry_uri.map(nerf_dart).unwrap_or_else(|| "//registry.npmjs.org/".into());

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied in 947bf6a. Renamed default_registry_uridefault_registry_url and spelled out in the doc that callers must pass the raw URL (including scheme), not a nerf-darted //host/.../ form.


Written by an agent (Claude Code, claude-opus-4-7).


Generated by Claude Code

Comment on lines +160 to +161
if !self.default_creds.is_empty()
&& let Some(header) = creds_to_header(&self.default_creds)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skipping this suggestion. After commit e8abefc, build_auth_headers deliberately inserts default-registry creds at the empty-string key, then relies on AuthHeaders::from_creds_map to do the nerf-darting on that key. This matches upstream's getAuthHeadersFromCreds convention (configByUri[''] holds defaults). Switching to from_map here would collapse the two-phase precedence and reintroduce the divergence fixed in bbec43a (where per-URI and default-registry creds for the same default registry produced non-deterministic winners). The double-build observation was correct for pre-e8abefc code where the caller pre-nerf-darted the default key.


Written by an agent (Claude Code, claude-opus-4-7).


Generated by Claude Code

Comment thread crates/registry/src/package.rs Outdated
Comment on lines +36 to +48
let url = || format!("{registry}{name}"); // TODO: use reqwest URL directly
let network_error = |error| NetworkError { error, url: url() };
http_client
.acquire()
.await
.get(url())
.header(
"accept",
"application/vnd.npm.install-v1+json; q=1.0, application/json; q=0.8, */*",
)
let mut request = http_client.acquire().await.get(url()).header(
"accept",
"application/vnd.npm.install-v1+json; q=1.0, application/json; q=0.8, */*",
);
// Mirrors `fetchMetadataFromFromRegistry` in pnpm v11's
// [`resolving/npm-resolver/src/fetch.ts`](https://github.com/pnpm/pnpm/blob/601317e7a3/resolving/npm-resolver/src/fetch.ts):
// resolve the per-URL `Authorization` value before issuing the
// request and attach it when present.
if let Some(value) = auth_headers.for_url(&url()) {
request = request.header("authorization", value);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied in 947bf6a.


Written by an agent (Claude Code, claude-opus-4-7).


Generated by Claude Code

Comment thread crates/registry/src/package_version.rs Outdated
Comment on lines +34 to +45
let url = || format!("{registry}{name}/{tag}");
let network_error = |error| NetworkError { error, url: url() };

http_client
.acquire()
.await
.get(url())
.header(
"accept",
"application/vnd.npm.install-v1+json; q=1.0, application/json; q=0.8, */*",
)
let mut request = http_client.acquire().await.get(url()).header(
"accept",
"application/vnd.npm.install-v1+json; q=1.0, application/json; q=0.8, */*",
);
// Same auth flow as `Package::fetch_from_registry` — see the
// doc comment there.
if let Some(value) = auth_headers.for_url(&url()) {
request = request.header("authorization", value);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied in 947bf6a.


Written by an agent (Claude Code, claude-opus-4-7).


Generated by Claude Code

claude added 2 commits May 13, 2026 17:51
Four Copilot findings on PR #337:

- `crates/cli/tests/install.rs:257` — doc referenced
  `crates/npmrc/src/api.rs`; the crate was renamed to
  `pacquet-config` in #420. Update to `crates/config/src/api.rs`.
- `crates/network/src/auth.rs:55` — `from_creds_map`'s
  `default_registry_uri` parameter docstring said "URI", which
  could be read as already-nerf-darted. Rename to
  `default_registry_url` and spell out: pass the raw URL, not a
  nerf-darted form (which would silently re-nerf to empty string
  and mask default creds).
- `crates/registry/src/package.rs:48` + `package_version.rs:45`
  — `url()` was a closure formatted three times (GET request,
  auth-header lookup, error mapper). Format once into a local
  `String` and reuse. Saves two formats and guarantees the auth
  lookup uses the byte-identical URL the request hits.

Skipped Copilot's `from_map` vs `from_creds_map` suggestion on
`npmrc_auth.rs:161`: that suggestion was correct for the pre-
e8abefc code, but `build_auth_headers` now inserts default creds
with an empty-string key (matching upstream's
`getAuthHeadersFromCreds` convention), so `from_creds_map` is
required to do the re-keying.
Round 5 review caught a real cardinal-rule divergence in
`AuthHeaders::from_creds_map`: the prior single-loop build wrote
both per-URI entries and the empty-key default into one HashMap.
When a `.npmrc` carried both `_authToken=A` (default) and
`//registry.npmjs.org/:_authToken=B` (per-URI for the default
registry), HashMap iteration order picked the winner, so the
final header was non-deterministic across runs.

Upstream's
[`getAuthHeadersFromCreds`](https://github.com/pnpm/pnpm/blob/601317e7a3/network/auth-header/src/getAuthHeadersFromConfig.ts)
processes per-URI entries first, then unconditionally overwrites
the default-registry slot with the default-creds header — the
default ALWAYS wins on its own URI. Split the build into two
phases to match.

Regression test
`default_creds_win_over_per_uri_on_default_registry` verified
with a break-then-run that swallows the second-phase insert.

Also reword the test docstring on
`install_resolves_env_var_in_npmrc_registry`: it doesn't actually
mirror the auth-token substitution in upstream's
`installing/deps-installer/test/install/auth.ts`. Spell out what
the test does cover (env-var resolution in the `registry=` line)
and what it doesn't (auth-header substitution).
@KSXGitHub KSXGitHub marked this pull request as ready for review May 13, 2026 18:14
Copilot AI review requested due to automatic review settings May 13, 2026 18:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 24 out of 25 changed files in this pull request and generated 1 comment.

Comment on lines +141 to +145
// joined with `/` it is `//host`; the loop slices through
// `parts[..i]` and re-joins, then appends a trailing slash.
for i in (3..=upper).rev() {
let key = format!("{}/", parts[..i].join("/"));
if let Some(value) = self.by_uri.get(&key) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied in ceea639. Changed the loop range to (3..upper).rev(), matching upstream's Math.min(parts.length, maxParts) - 1 literally. Kept the existing split('/') so parts.len() stays byte-equivalent to upstream's parts.length; only the bound moves. Same observable effect as switching to split_terminator('/') would have, but matches the upstream loop exactly.


Written by an agent (Claude Code, claude-opus-4-7).


Generated by Claude Code

Upstream's
[`getAuthHeaderByURI`](https://github.com/pnpm/pnpm/blob/601317e7a3/network/auth-header/src/index.ts)
walks `for (let i = Math.min(parts.length, maxParts) - 1; i >= 3; i--)`
— exclusive upper bound. Pacquet's was `(3..=upper).rev()`
(inclusive), running one extra iteration whose key always ended
in `//` (the trailing empty segment from `nerfed.split('/')` plus
the appended `/`). The extra lookup never matched anything, but
the loop bound diverged from upstream literally and burned a
HashMap lookup per call.

Switch to `(3..upper).rev()` to match upstream exactly. Caught
independently by Copilot (current PR) and earlier by KSXGitHub's
review thread `r3167803912`-adjacent comment.
@zkochan zkochan merged commit d8e7807 into main May 13, 2026
23 of 24 checks passed
@zkochan zkochan deleted the claude/support-proper-auth-8NRX6 branch May 13, 2026 18:39
zkochan added a commit that referenced this pull request May 13, 2026
Ports pnpm v11's `getDispatcher` (`network/fetch/src/dispatcher.ts` at
SHA 94240bc046) onto reqwest, with the resolved proxy configuration
modeled by the new `ProxyConfig` + `NoProxySetting` types in
`pacquet-network`. HTTPS targets route through `https_proxy`; HTTP
targets through `http_proxy`; the `no_proxy` field short-circuits both
via a reverse-dot-segment-prefix matcher that mirrors upstream's
`checkNoProxy` semantics (`npmjs.org` matches `registry.npmjs.org` but
not `evilnpmjs.org`).

The proxy types live in `pacquet-network` rather than `pacquet-config`
because `pacquet-config` already depends on `pacquet-network` for the
auth-headers plumbing (#337), so the reverse direction would form a
cycle. The downstream `pacquet-config` will hold a
`Config.proxy: ProxyConfig` and populate it from `.npmrc` + env in the
follow-up commit.

Basic-auth userinfo embedded in the proxy URL is stripped and
percent-decoded before being forwarded via `Proxy::basic_auth`, matching
pnpm's `decodeURIComponent(user):decodeURIComponent(pass)` step. The
percent-decoder is a 15-line inline helper rather than a new direct
`percent-encoding` dep, since it only runs against the two halves of a
proxy userinfo. Unlike JavaScript's `decodeURIComponent`, which throws
on malformed sequences, the helper keeps invalid `%XX` escapes verbatim
— the safer behavior in a config path where the alternative would be
rejecting a half-broken password.

`parse_proxy_url` retries failed parses with an `http://` prefix to
support shorthand values like `proxy.example:8080`, matching pnpm's
`parseProxyUrl`. Rust's `url` parser is permissive enough to accept the
shorthand as scheme + opaque path, so the parser also rejects any first-
attempt parse that lacks a host — forcing the retry through that
authority-aware path.

Invalid proxy URLs surface as `ProxyError::InvalidProxy` with diagnostic
code `ERR_PNPM_INVALID_PROXY`, matching upstream's error code. Failure
is detected eagerly at client-build time (same as pnpm's
`getProxyAgent`) rather than lazily per-request.

Enables reqwest's `socks` feature so socks4/socks4a/socks5 URLs are
honored — pnpm supports the same set via its socks-client wrapper.

The existing `new_for_installs()` is preserved as a thin wrapper around
`for_installs(&ProxyConfig::default())` so test fixtures and the
benchmark harness keep their call sites unchanged.

Tests port the unit-testable describe blocks from
`network/fetch/test/dispatcher.test.ts`: per-URL routing, basic-auth
decoding, noProxy reverse-dot-prefix match, bypass-all literal, invalid
URL diagnostic code, and SOCKS-URL parse smoke. Two mockito integration
tests cover end-to-end HTTP proxy forwarding (with decoded
`Proxy-Authorization`) and the noProxy-bypass path.
zkochan added a commit that referenced this pull request May 13, 2026
* feat(network): add ThrottledClient::for_installs with proxy support

Ports pnpm v11's `getDispatcher` (`network/fetch/src/dispatcher.ts` at
SHA 94240bc046) onto reqwest, with the resolved proxy configuration
modeled by the new `ProxyConfig` + `NoProxySetting` types in
`pacquet-network`. HTTPS targets route through `https_proxy`; HTTP
targets through `http_proxy`; the `no_proxy` field short-circuits both
via a reverse-dot-segment-prefix matcher that mirrors upstream's
`checkNoProxy` semantics (`npmjs.org` matches `registry.npmjs.org` but
not `evilnpmjs.org`).

The proxy types live in `pacquet-network` rather than `pacquet-config`
because `pacquet-config` already depends on `pacquet-network` for the
auth-headers plumbing (#337), so the reverse direction would form a
cycle. The downstream `pacquet-config` will hold a
`Config.proxy: ProxyConfig` and populate it from `.npmrc` + env in the
follow-up commit.

Basic-auth userinfo embedded in the proxy URL is stripped and
percent-decoded before being forwarded via `Proxy::basic_auth`, matching
pnpm's `decodeURIComponent(user):decodeURIComponent(pass)` step. The
percent-decoder is a 15-line inline helper rather than a new direct
`percent-encoding` dep, since it only runs against the two halves of a
proxy userinfo. Unlike JavaScript's `decodeURIComponent`, which throws
on malformed sequences, the helper keeps invalid `%XX` escapes verbatim
— the safer behavior in a config path where the alternative would be
rejecting a half-broken password.

`parse_proxy_url` retries failed parses with an `http://` prefix to
support shorthand values like `proxy.example:8080`, matching pnpm's
`parseProxyUrl`. Rust's `url` parser is permissive enough to accept the
shorthand as scheme + opaque path, so the parser also rejects any first-
attempt parse that lacks a host — forcing the retry through that
authority-aware path.

Invalid proxy URLs surface as `ProxyError::InvalidProxy` with diagnostic
code `ERR_PNPM_INVALID_PROXY`, matching upstream's error code. Failure
is detected eagerly at client-build time (same as pnpm's
`getProxyAgent`) rather than lazily per-request.

Enables reqwest's `socks` feature so socks4/socks4a/socks5 URLs are
honored — pnpm supports the same set via its socks-client wrapper.

The existing `new_for_installs()` is preserved as a thin wrapper around
`for_installs(&ProxyConfig::default())` so test fixtures and the
benchmark harness keep their call sites unchanged.

Tests port the unit-testable describe blocks from
`network/fetch/test/dispatcher.test.ts`: per-URL routing, basic-auth
decoding, noProxy reverse-dot-prefix match, bypass-all literal, invalid
URL diagnostic code, and SOCKS-URL parse smoke. Two mockito integration
tests cover end-to-end HTTP proxy forwarding (with decoded
`Proxy-Authorization`) and the noProxy-bypass path.

* feat(config): parse proxy keys from .npmrc with env-var fallback cascade

Adds `Config.proxy: ProxyConfig` (the type lives in `pacquet-network`,
see preceding commit) and extends `NpmrcAuth` to capture the four
proxy keys (`https-proxy`, `http-proxy`, `proxy`, `no-proxy`, `noproxy`)
plus the env-var fallback cascade pnpm 11 runs in
`config/reader/src/index.ts:591-600` (SHA 94240bc046). The cascade now
runs unconditionally from `Config::current` — even when no `.npmrc` is
present — so the env fallback fires the same way it does in pnpm.

The new `NpmrcAuth::apply_proxy_cascade::<Api: EnvVar>` is generic over
the project-wide `EnvVar` capability trait (introduced by #339), so
cascade unit tests inject the env without taking `EnvGuard`'s global
lock. The existing `apply_to` test helper now runs the full three-phase
sequence (`apply_registry_and_warn` → `apply_proxy_cascade` →
`build_auth_headers`).

`no-proxy=true` (literal) is upstream's `noProxy: string | true`
"bypass every proxy" shape and is parsed as `NoProxySetting::Bypass`.
Comma-separated host lists become `NoProxySetting::List`, trimmed with
empties dropped — the network layer reverse-dot-prefix-matches against
`List` entries when applying the cascade.

The cascade is invoked from `Config::current` between
`apply_registry_and_warn` (phase 1) and `build_auth_headers` (phase 2)
of the existing auth flow. Phase placement is incidental — the proxy
cascade is independent of the registry and creds layers — but slotting
it there keeps every `.npmrc`-consuming step in one block of the
function for the reader.

Tests cover the parse arms (each proxy key, the `no-proxy`/`noproxy`
last-wins alias), the cascade branches (legacy `proxy` → https slot,
http inheriting resolved https, env fallback only when .npmrc unset,
.npmrc winning over env, `PROXY` env fallback, lowercase-only env), and
the `noProxy: true` → `Bypass` parse. A `static_env!` macro keeps each
test's env-table inline. A real-`std::env::var` smoke test in `lib.rs`
exercises the path through `Config::current` under `EnvGuard`.

The `Config` literal in
`crates/package-manager/src/install_package_from_registry/tests.rs`
gains the `proxy` field (defaults to `ProxyConfig::default()`).

* feat(cli): wire proxy config through State::init

Switches `crates/cli/src/state.rs` from `ThrottledClient::new_for_installs()`
to `ThrottledClient::for_installs(&config.proxy)` so the install client
honors the `.npmrc` / env proxy cascade landed in the preceding two
commits. Proxy build failures surface as a new `InitStateError::Proxy`
variant carrying `ProxyError` (transparently diagnosed as
`ERR_PNPM_INVALID_PROXY`).

Drops the `Load` prefix on `InitStateError`'s variants
(`LoadManifest` → `Manifest`, `LoadLockfile` → `Lockfile`) so clippy's
`enum_variant_names` lint stops firing once a third `Load*`-prefixed
variant pushes the type past its shared-prefix threshold. The variants
are still self-descriptive inside `InitStateError::*`; no public
consumers exist outside `state.rs`.

Folds in `cargo fmt` reflows of three test files
(`crates/config/src/npmrc_auth/tests.rs`, `crates/network/src/lib.rs`,
`crates/network/src/tests.rs`) — trivial line-joins on lines that just
fit under the 100-column budget.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support proper auth

4 participants