feat: auth#337
Conversation
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)
This comment was marked as resolved.
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\``.
91acdac to
f73350b
Compare
Codecov Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
Micro-Benchmark ResultsLinux |
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
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)
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
]
}
]
} |
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
|
@zkochan — heads up that the test-the-tests practice from The reason is sandbox-side: 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.
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 |
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.
This comment was marked as resolved.
0e5a3e4 to
fd2d44a
Compare
`<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.
fd2d44a to
b15f887
Compare
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.
# Conflicts: # crates/npmrc/src/npmrc_auth.rs
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.
There was a problem hiding this comment.
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
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
crates/cli/src/cli_args.rscrates/cli/tests/install.rscrates/config/Cargo.tomlcrates/config/src/api.rscrates/config/src/env_replace.rscrates/config/src/lib.rscrates/config/src/npmrc_auth.rscrates/config/src/npmrc_auth/tests.rscrates/config/src/workspace_yaml/tests.rscrates/network/Cargo.tomlcrates/network/src/auth.rscrates/network/src/lib.rscrates/package-manager/src/add.rscrates/package-manager/src/install_package_by_snapshot.rscrates/package-manager/src/install_package_from_registry.rscrates/package-manager/src/install_package_from_registry/tests.rscrates/registry/Cargo.tomlcrates/registry/src/package.rscrates/registry/src/package/tests.rscrates/registry/src/package_version.rscrates/tarball/src/lib.rscrates/tarball/src/tests.rsplans/TEST_PORTING.mdtasks/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 andpipe-traitchains; do not break them into intermediateletbindings 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 (&Pathover&PathBuf,&strover&String) when it does not force extra copies.
PreferArc::clone(&x)andRc::clone(&x)overx.clone()for reference-counted types to make the cost visible at the call site.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse 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::*;inlib.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 plainStringor&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 viaTryFrom<String>and/orFromStr; 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 infallibleFrom<String>andFrom<&str>constructor in the Rust wrapper.
When upstream TypeScript occasionally constructs a branded type without validation (via bareasassertion), add afrom_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.rscrates/package-manager/src/install_package_from_registry/tests.rstasks/micro-benchmark/src/main.rscrates/package-manager/src/install_package_by_snapshot.rscrates/registry/src/package/tests.rscrates/config/src/workspace_yaml/tests.rscrates/cli/tests/install.rscrates/registry/src/package.rscrates/network/src/lib.rscrates/package-manager/src/install_package_from_registry.rscrates/registry/src/package_version.rscrates/cli/src/cli_args.rscrates/config/src/api.rscrates/network/src/auth.rscrates/config/src/npmrc_auth/tests.rscrates/config/src/env_replace.rscrates/tarball/src/lib.rscrates/config/src/npmrc_auth.rscrates/config/src/lib.rscrates/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 inplans/TEST_PORTING.mdbefore adding ported tests. Follow the conventions expected for ports: useknown_failuresmodules, usepacquet_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, usedbg!for complex structures, skip logging for simple scalarassert_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.rscrates/package-manager/src/install_package_from_registry/tests.rstasks/micro-benchmark/src/main.rscrates/package-manager/src/install_package_by_snapshot.rscrates/registry/src/package/tests.rscrates/config/src/workspace_yaml/tests.rscrates/cli/tests/install.rscrates/registry/src/package.rscrates/network/src/lib.rscrates/package-manager/src/install_package_from_registry.rscrates/registry/src/package_version.rscrates/cli/src/cli_args.rscrates/config/src/api.rscrates/network/src/auth.rscrates/config/src/npmrc_auth/tests.rscrates/config/src/env_replace.rscrates/tarball/src/lib.rscrates/config/src/npmrc_auth.rscrates/config/src/lib.rscrates/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.rscrates/registry/src/package/tests.rscrates/config/src/workspace_yaml/tests.rscrates/config/src/npmrc_auth/tests.rscrates/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 valueConsider reducing visibility to
pub(crate).
from_mapis only called once within this crate (byfrom_creds_map). Per the project's public API minimality principle, it could bepub(crate)to avoid expanding the public surface unnecessarily.
125-125: 💤 Low valueLoop range has an off-by-one relative to upstream.
Upstream's
getAuthHeaderByURIloop isfor (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 valueClarify IPv6 comment.
The comment says "Skip IPv6 brackets" but the code doesn't skip them—it treats
[::1]:8080as the entire host (withport = 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_passworddecode fallback still diverges from pnpm auth parsing.Line 195 still falls back to raw
_passwordwhen 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!
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`.
There was a problem hiding this comment.
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
.npmrcauth parsing inpacquet-config(default + per-registry creds) with${VAR}/${VAR:-default}substitution via DI (EnvVar/RealApi). - Introduced
pacquet-network::AuthHeadersURL-keyed lookup (nerf-dart + longest-prefix matching + basic-auth-in-URL precedence). - Threaded
AuthHeadersthrough 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.
| /// 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 |
There was a problem hiding this comment.
| /// 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()); |
There was a problem hiding this comment.
Applied in 947bf6a. Renamed default_registry_uri → default_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
| if !self.default_creds.is_empty() | ||
| && let Some(header) = creds_to_header(&self.default_creds) |
There was a problem hiding this comment.
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
| 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); | ||
| } |
There was a problem hiding this comment.
| 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); | ||
| } |
There was a problem hiding this comment.
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).
| // 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) { |
There was a problem hiding this comment.
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.
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(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.
Resolves #336.
Summary
Ports pnpm v11's auth flow so
pacquet installcan talk to private registries on the same footing aspnpm install. Pacquet now parses credentials out of.npmrc, expands${VAR}references against the process environment, and attaches the rightAuthorizationheader 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_passwordkeys, both default-registry (bare) and per-registry (//host[:port]/path/:_authToken=…family), parsed incrates/config(formerlycrates/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 atracing::warn!and leave the value verbatim — same best-effort behaviour as pnpm'ssubstituteEnv.AuthHeaders) incrates/network, mirroringcreateGetAuthHeaderByURI: nerf-darts the request URL, walks parent path prefixes from longest to host-only, falls back to a port-stripped lookup matching upstream'sremovePort(any port, not just protocol defaults), and prefers inlineuser:password@basic auth when present.apply_registry_and_warn/build_auth_headerssplit insideConfig::currentensurespnpm-workspace.yaml'sregistryoverride propagates to the bearer-token nerf-dart key.Authorizationheader is attached onPackage::fetch_from_registry,PackageVersion::fetch_from_registry, and insideDownloadTarballToStore's retry loop.Test porting checklist
Boxes ticked in
plans/TEST_PORTING.mdfor the upstream auth tests this PR ports:network/auth-header/test/getAuthHeadersFromConfig.test.ts—should 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.ts—authToken,authPairBase64,authUsername and authPassword.The remaining auth checkboxes (
tokenHelper,pnpm auth fileprecedence, redirect header-stripping, theinstalling/deps-installer/test/install/auth.tsintegration tests) need features pacquet doesn't yet have — token helpers, theauth.inilayer, scoped registries, redirect handling — and stay open for follow-ups.Notes / scope
always-authis intentionally not honoured. pnpm v11 doesn't either — the auth header is selected by URL match alone, andalways-authwas deprecated upstream.@scope:registrykeys remain unparsed; the parser silently ignores them, matching the pre-Support proper auth #336 behaviour. Thecreds_by_urishape is ready to grow as those land.crates/tarballwas 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.pub trait EnvVar+pub struct RealApiincrates/config/src/api.rs, threaded throughenv_replace,NpmrcAuth::from_ini, andConfig::currentunder oneApi: EnvVarbound. Production callers turbofishRealApiexplicitly: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(theregistry-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
Tests