feat(config,network,cli): TLS keys + local-address from .npmrc by zkochan · Pull Request #490 · pnpm/pacquet

zkochan · 2026-05-13T20:25:21Z

Closes #482.

Summary

Ports pnpm v11's TLS + local-address .npmrc keys onto pacquet
(SHA 94240bc046),
the natural pair to the proxy support that landed in #476. Three layers:

feat(network) (e9ed56c): adds TlsConfig next to ProxyConfig in pacquet-network, with ca: Vec<String>, cert/key, strict_ssl: Option<bool>, and local_address: Option<IpAddr>. ThrottledClient::for_installs gains a &TlsConfig parameter; the unified error surface is the new ForInstallsError enum carrying either ProxyError or TlsError. CAs route through Certificate::from_pem, client identities through Identity::from_pkcs8_pem (the only PKCS path reqwest exposes on the native-tls backend pacquet builds with). strict_ssl defaults to true at build site, matching pnpm's per-emit-site strictSsl ?? true rather than a config-layer default.
feat(config) (e8dcd87): extends NpmrcAuth with the six new keys (ca, cafile, cert, key, strict-ssl, local-address) and adds apply_tls_and_local_address to populate Config.tls. cafile reads from disk and splits on -----END CERTIFICATE----- to mirror pnpm's loadCAFile. Unreadable cafile is silently treated as unset. Invalid strict-ssl and local-address values drop silently.
feat(cli) (1759f9d): one-line swap in State::init to pass &config.tls instead of TlsConfig::default(). Folds in two CI-parity fixups: the self-signed test cert moves to a shared .pem fixture under crates/network/tests/fixtures/ (typos linter false positives on base64 DER), and four reqwest intra-doc links become plain backticks.

Parity policy

Faithful to pnpm — see the research brief on the issue. Highlights:

No new error codes. pnpm doesn't define ERR_PNPM_INVALID_CA etc.; invalid PEMs surface as raw tls.connect errors at request time upstream. Pacquet validates eagerly via Certificate::from_pem / Identity::from_pkcs8_pem (pushing the failure to per-request time would silently degrade every install behind a broken ca) but deliberately omits a code(...) attribute on TlsError so reviewers can see at a glance it's a pacquet-only diagnostic, not a pnpm error code.
Silent cafile-not-found. Matches pnpm's catch {} swallow in loadCAFile.
No env-var fallback. pnpm reads only .npmrc; Node's implicit NODE_EXTRA_CA_CERTS / NODE_TLS_REJECT_UNAUTHORIZED honoring doesn't apply to pacquet's reqwest stack.
strict-ssl: false disables both chain-of-trust and hostname verification, matching Node's rejectUnauthorized=false short-circuit (pacquet uses reqwest's danger_accept_invalid_certs(true) which has the same combined semantics).

Reviewer flags

PKCS#8-only client keys. Reqwest's native-tls backend exposes only Identity::from_pkcs8_pem; legacy PKCS#1 keys (-----BEGIN RSA PRIVATE KEY-----) and PKCS#12 bundles are not supported by this constructor. Documented at the apply_tls callsite with the openssl pkcs8 -topk8 -nocrypt conversion command. Switching to rustls-tls would broaden the supported formats but is out of scope here.
Per-registry TLS overrides (//host:cafile=, //host:ca=, //host:cert=, //host:key=) are not included. Same shape as the existing scoped-auth handling but a sizeable feature on its own; flagged in feat(config,network): TLS keys + local-address from .npmrc #482 as a follow-up.

Test plan

cargo nextest run -p pacquet-network — 185 tests pass (50 network + 135 config; 17 new TLS tests added across the two crates)
cargo nextest run --workspace — 1085 tests pass
just ready — typos, fmt, check, lint clean
taplo format --check clean
RUSTDOCFLAGS=-D warnings cargo doc --no-deps --workspace --document-private-items clean
just dylint clean

Upstream coverage

Ports the unit-testable subset of pnpm's TLS handling — CA parse (valid + multi + index-tagged invalid), client-identity parse, strict-ssl on/off/unset, local-address pin, cafile multi-cert split, cafile-not-found silent-drop, invalid local-address silent-drop, and strict-ssl invalid-token silent-drop. The Identity/CA error paths exercise the same Certificate::from_pem / Identity::from_pkcs8_pem code paths reqwest uses at request time.

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

Summary by CodeRabbit

New Features
- Added TLS and local-address resolution from .npmrc into resolved configuration (CA certs inline or file, client cert/key, strict-ssl, outbound local-address).
- Network client now applies TLS and proxy settings together and exposes a single unified network error surface.
Tests
- Extensive unit and integration tests covering .npmrc parsing, CA file handling, strict-ssl/local-address behaviors, client identity, and network client build/error cases.

…or_installs Adds `TlsConfig` (CA list, client cert/key, strict_ssl, local_address) to `pacquet-network` alongside the existing `ProxyConfig`, and extends `ThrottledClient::for_installs` to accept it. The unified error surface is the new `ForInstallsError` enum carrying either `ProxyError` or `TlsError`, so callers handle one error type for the whole client constructor. Parity policy follows the upstream brief (pnpm v11, SHA 94240bc046): - `strict_ssl` is `Option<bool>` and applied as `unwrap_or(true)` at build time, matching pnpm's per-emit-site default in `network/fetch/src/dispatcher.ts:191,197,241,295` rather than a config-layer default. This keeps "explicitly false" distinguishable from "unset" for downstream config-layering consumers. - `local_address` is `Option<IpAddr>`. pnpm hands the value to undici as a bare string with no validation; pacquet parses early so an invalid value diagnoses at config-load time instead of mid-install. - `ca` is a `Vec<String>` (one PEM per entry). The `cafile` loader in the config layer (next commit) will split on `-----END CERTIFICATE-----` to mirror pnpm's `config/reader/src/loadNpmrcFiles.ts:249-265`. - Client `cert` + `key` are routed through `Identity::from_pkcs8_pem` — the only PKCS path reqwest exposes on its native-tls backend. PKCS#1 keys (`-----BEGIN RSA PRIVATE KEY-----`) are rejected; a doc comment on `apply_tls` explains the workaround (`openssl pkcs8 -topk8 -nocrypt`) for users hitting that limit. - No `ERR_PNPM_INVALID_CA` / `_CERT` / `_KEY` codes exist upstream — invalid PEMs surface as raw `tls.connect` errors at request time in pnpm. Pacquet validates eagerly via `Certificate::from_pem` / `Identity::from_pkcs8_pem` because pushing the failure to per-request time would silently degrade every install behind a broken `ca`, but the diagnostics deliberately omit a `code(...)` attribute so a reviewer can see at a glance that this is a pacquet-only diagnostic, not a pnpm error code. - No env-var fallback. pnpm reads only `.npmrc`; Node's implicit `NODE_EXTRA_CA_CERTS` / `NODE_TLS_REJECT_UNAUTHORIZED` honoring doesn't apply to pacquet's reqwest stack. The existing `new_for_installs()` and `for_installs(&proxy)` callers are updated: `new_for_installs()` now defaults both `ProxyConfig` and `TlsConfig`, and `crates/cli/src/state.rs` passes `TlsConfig::default()` for now (the follow-up commit wires `config.tls` through). `InitStateError`'s `Proxy` variant becomes `Network` so it can carry the unified `ForInstallsError`. Tests cover: CA parse (valid + multiple + index-tagged invalid), client-identity parse failure, strict-ssl off and unset paths, and a local-address pin. The TLS unit tests share a generated self-signed cert (regenerable via the `openssl req` line in the constant comment).

Extends `NpmrcAuth` to capture the six `.npmrc` keys covered by pnpm v11's TLS allow-list (`ca`, `cafile`, `cert`, `key`, `strict-ssl`, `local-address`) and applies them through a new `NpmrcAuth::apply_tls_and_local_address` step in `Config::current`, populating the `Config.tls: pacquet_network::TlsConfig` field added in the preceding commit. Parity policy follows the upstream brief (pnpm v11, SHA 94240bc046): - `ca` parses as repeated values into a `Vec<String>`, mirroring upstream's `[null, String, Array]` nopt shape where multiple `ca=` lines arrive as an array (`config/reader/src/npmConfigTypes.ts:11-67`). - `cafile=<path>` reads from disk at apply time and splits on `-----END CERTIFICATE-----` to produce one PEM per certificate — same shape as pnpm's `loadCAFile` (`config/reader/src/loadNpmrcFiles.ts:238-265`). Unreadable `cafile` is silently treated as unset (matches upstream's `catch {}` swallow). Inline `ca` entries appear before `cafile`- loaded ones in the final list, matching pnpm's concatenation order. - `strict-ssl` accepts only the literal `true` / `false` tokens (pnpm/nopt drops anything else); the value lands as `Option<bool>` and the `true` default is applied at the network client-build site, not here. - `local-address` is parsed as `std::net::IpAddr` at apply time. An invalid value is silently dropped — pnpm hands the raw string to undici with no validation and lets Node error at connect time; pacquet validates eagerly but mirrors the silent-drop policy. - Per-registry TLS scoping (`//host:cafile=`, `//host:ca=`, `//host:cert=`, `//host:key=`) is **not** covered here. Same shape as the existing scoped-auth handling but a sizeable feature on its own; tracked separately. The CLI continues to pass `TlsConfig::default()` for now — the follow-up commit wires `config.tls` through `State::init` so the fields actually reach the install client. Tests cover: parse arms for every new key, `strict-ssl` rejection of invalid tokens, `local-address` `IpAddr` round-trip, invalid `local-address` silent-drop, `cafile` multi-cert split, missing `cafile` silent-drop, inline `ca` + `cafile` concatenation order, and the empty-default round-trip. The `apply_to` test helper now also runs `apply_tls_and_local_address`, so every existing test that constructs a `Config` via `apply_to` exercises the new step.

Switches `crates/cli/src/state.rs` from `ThrottledClient::for_installs(&config.proxy, &TlsConfig::default())` to `ThrottledClient::for_installs(&config.proxy, &config.tls)` so the TLS keys parsed in the preceding commit (`ca`, `cafile`, `cert`, `key`, `strict-ssl`, `local-address`) reach the install client. Folds in two CI-parity fixups that aren't worth their own commits: - The self-signed test cert moves from inline `const` strings in `crates/network/src/tests.rs` and `crates/config/src/npmrc_auth/tests.rs` into a shared fixture at `crates/network/tests/fixtures/test-ca.pem`, loaded via `include_str!`. The base64-encoded DER body otherwise triggers typos-linter false positives ("UE" → "USE", "OT" → "TO", etc.) — keeping the cert in a `.pem` file removes those tokens from the source-code word list while still letting the constant resolve at compile time. - Four intra-doc links to reqwest types (`Identity::from_pem`, `danger_accept_invalid_certs`, `local_address`, `Certificate::from_pem`) become plain backtick-quoted names. They were unresolved under `RUSTDOCFLAGS=-D warnings cargo doc --no-deps --workspace --document-private-items` because reqwest isn't a public-doc dep of `pacquet-network`.

coderabbitai · 2026-05-13T20:25:34Z

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c1770f91-016c-4dce-b2e0-4978bb6c9910

📥 Commits

Reviewing files that changed from the base of the PR and between c74f93b and cde2ff5.

📒 Files selected for processing (2)

crates/config/src/npmrc_auth.rs
crates/config/src/npmrc_auth/tests.rs

📝 Walkthrough

Walkthrough

Parses .npmrc TLS/local-address into NpmrcAuth, resolves them into Config.tls, adds TlsConfig/TlsError, applies TLS/local-address in ThrottledClient::for_installs (now returns ForInstallsError), and wires the config into CLI and tests.

Changes

TLS and local-address configuration from .npmrc

Layer / File(s)	Summary
TLS configuration data model and errors `crates/network/src/tls.rs`, `crates/network/src/tls/tests.rs`	`TlsConfig` holds resolved CA PEMs, optional client cert/key, tri-state `strict_ssl`, and `local_address: Option<IpAddr>`; `TlsError` reports invalid CA entries (by index) and invalid client identity.
Network client TLS application and error wrapping `crates/network/src/lib.rs`	`ThrottledClient::for_installs` now accepts `&TlsConfig` and returns `Result<_, ForInstallsError>`; `apply_tls` maps `TlsConfig` onto `reqwest::ClientBuilder` (add_root_certificate, identity, danger_accept_invalid_certs, local_address). `ForInstallsError` wraps `ProxyError` and `TlsError`.
Network client tests for TLS configuration and validation `crates/network/src/tests.rs`	Existing proxy tests updated to pass `TlsConfig::default()`; added tests covering valid/multiple CA PEMs, malformed CA indexing, `strict_ssl` behavior, `local_address` pinning, and client identity parsing errors.
npmrc TLS and local-address field parsing `crates/config/src/npmrc_auth.rs`	`NpmrcAuth` gains `ca`, `cafile`, `cert`, `key`, `strict_ssl`, and `local_address`; `from_ini` parses these keys (accumulates repeated `ca`), and `apply_tls_and_local_address` resolves inline+file CAs, sets cert/key/strict_ssl, and parses `local-address` into `IpAddr`.
npmrc TLS and local-address parsing tests `crates/config/src/npmrc_auth/tests.rs`	Adds parsing and application tests for `ca`/`cafile`/`cert`/`key`/`strict-ssl`/`local-address`, cafile splitting, missing-cafile behavior, invalid local-address handling, and `NpmrcAuth::default()` expectations.
Config struct and Config::current integration `crates/config/src/lib.rs`	`Config` gains `pub tls: pacquet_network::TlsConfig`; `Config::current` docs updated and calls `npmrc_auth.apply_tls_and_local_address(&mut config)` during `.npmrc` application.
CLI state initialization with TLS support `crates/cli/src/state.rs`, `crates/package-manager/src/install_package_from_registry/tests.rs`	`State::init` passes `&config.tls` to `ThrottledClient::for_installs`, `InitStateError::Proxy` → `InitStateError::Network(ForInstallsError)`, and test helper initializes `tls: Default::default()`.

Sequence Diagram

sequenceDiagram
  participant NpmrcAuth
  participant Config
  participant State
  participant ThrottledClient
  participant ReqwestBuilder
  NpmrcAuth->>Config: apply_tls_and_local_address(&mut config)
  State->>ThrottledClient: ThrottledClient::for_installs(&config.proxy, &config.tls)
  ThrottledClient->>ReqwestBuilder: apply_tls(builder, tls)

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~40 minutes

Possibly related PRs

pnpm/pacquet#420: Prior config/.npmrc refactor that this PR extends with TLS/local-address parsing and application.
pnpm/pacquet#476: Earlier proxy support that changed for_installs and State::init; this PR continues that API evolution by adding TLS and ForInstallsError wrapping.

"🐰 I read .npmrc under moonlight's gloss,
I bundle CA roots and keys across,
strict-ssl checked, local route found,
the client builds safe, the requests go 'round."

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'feat(config,network,cli): TLS keys + local-address from .npmrc' directly and concisely describes the main change: adding TLS and local-address configuration from .npmrc files across three crates. It is specific, clear, and accurately reflects the PR's primary objective.
Description check	✅ Passed	The PR description provides a comprehensive summary, references the closed issue (`#482`), includes upstream references with commit SHAs, details implementation across three crates, explains parity with pnpm semantics, documents limitations (PKCS#8-only, no per-registry scoping), and provides a complete test plan showing all CI checks passing.
Linked Issues check	✅ Passed	The PR successfully implements all coding requirements from `#482`: extends NpmrcAuth with six new .npmrc TLS keys, introduces TlsConfig with proper field types, wires TlsConfig into ThrottledClient::for_installs with unified ForInstallsError, implements Certificate/Identity parsing via reqwest, provides strict_ssl and local_address handling with correct semantics, includes cafile filesystem reading with silent-drop behavior, and adds 17 new TLS-focused unit tests covering all specified scenarios (CA parsing, client identity, strict_ssl, local_address, cafile splitting, invalid value dropping).
Out of Scope Changes check	✅ Passed	All changes are directly aligned with `#482` requirements: TLS key parsing (ca, cafile, cert, key, strict-ssl, local-address), TlsConfig introduction, reqwest builder wiring, error handling, and comprehensive test coverage. The PR explicitly excludes out-of-scope items (CLI flags, per-registry TLS overrides, PKCS#1/PKCS#12 support) as documented in the PR description.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch feat/tls-keys-local-address

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 2

🤖 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/config/src/npmrc_auth.rs`:
- Around line 204-210: The "strict-ssl" match arm only sets auth.strict_ssl when
parse_bool(&value) returns Some, leaving a prior value intact on parse failures;
modify the "strict-ssl" handling so that if parse_bool(&value) yields
Some(parsed) you set auth.strict_ssl = Some(parsed), and otherwise you
explicitly clear it (auth.strict_ssl = None) so invalid reassignments like
"strict-ssl=oops" reset to unset/default behavior; update the match arm around
parse_bool and auth.strict_ssl accordingly.

In `@crates/network/src/tls.rs`:
- Around line 89-90: The doc comment on the InvalidClientIdentity variant refers
to the wrong API; update the text to mention Identity::from_pkcs8_pem instead of
Identity::from_pem so the documentation matches the actual call site (look for
the InvalidClientIdentity enum/variant and its doc comment in tls.rs) — simply
edit the comment line that currently says `Identity::from_pem` to read
`Identity::from_pkcs8_pem`.

🪄 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: 9ca956bb-6a02-4c9a-85da-19f3b69195eb

📥 Commits

Reviewing files that changed from the base of the PR and between 5397931 and 1759f9d.

⛔ Files ignored due to path filters (1)

crates/network/tests/fixtures/test-ca.pem is excluded by !**/*.pem

📒 Files selected for processing (9)

crates/cli/src/state.rs
crates/config/src/lib.rs
crates/config/src/npmrc_auth.rs
crates/config/src/npmrc_auth/tests.rs
crates/network/src/lib.rs
crates/network/src/tests.rs
crates/network/src/tls.rs
crates/network/src/tls/tests.rs
crates/package-manager/src/install_package_from_registry/tests.rs

📜 Review details

🧰 Additional context used

📓 Path-based instructions (1)

**/*.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/install_package_from_registry/tests.rs
crates/network/src/tls/tests.rs
crates/cli/src/state.rs
crates/config/src/lib.rs
crates/network/src/tls.rs
crates/config/src/npmrc_auth.rs
crates/config/src/npmrc_auth/tests.rs
crates/network/src/tests.rs
crates/network/src/lib.rs

🧠 Learnings (5)

📚 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/network/src/tls/tests.rs
crates/config/src/npmrc_auth/tests.rs
crates/network/src/tests.rs

📚 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/install_package_from_registry/tests.rs
crates/network/src/tls/tests.rs
crates/cli/src/state.rs
crates/config/src/lib.rs
crates/network/src/tls.rs
crates/config/src/npmrc_auth.rs
crates/config/src/npmrc_auth/tests.rs
crates/network/src/tests.rs
crates/network/src/lib.rs

📚 Learning: 2026-05-13T19:22:48.951Z

Learnt from: zkochan
Repo: pnpm/pacquet PR: 478
File: crates/package-manager/src/hoisted_dep_graph.rs:51-55
Timestamp: 2026-05-13T19:22:48.951Z
Learning: When reviewing this Rust codebase, avoid introducing/using a newtype like `PkgIdWithPatchHash` in only one module (e.g., `hoisted_dep_graph.rs`) if other related pacquet modules still represent the upstream pnpm “pkg id with patch hash” as a plain `String` (e.g., `virtual_store_layout`). For type consistency, either keep `pkg_id_with_patch_hash` as `String` here, or require a workspace-wide sweep that defines/extracts the `PkgIdWithPatchHash` newtype once and updates all call sites together (otherwise defer the refactor to a coordinated follow-up PR).

Applied to files:

crates/package-manager/src/install_package_from_registry/tests.rs

📚 Learning: 2026-05-13T20:09:22.171Z

Learnt from: zkochan
Repo: pnpm/pacquet PR: 486
File: crates/package-manager/src/hoisted_dep_graph.rs:472-476
Timestamp: 2026-05-13T20:09:22.171Z
Learning: In pnpm/pacquet, when generating serialized `hoisted_locations` strings (used for `.modules.yaml`), normalize path separators to forward slashes (e.g., replace `\\` with `/`). This preserves cross-platform portability: the output must not use OS-native separators because upstream pnpm’s `path.relative` can return platform-specific separators, which would break consistency with pnpm/pacquet’s other serialized formats on Windows.

Applied to files:

crates/package-manager/src/install_package_from_registry/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 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/network/src/tests.rs

🔇 Additional comments (7)

crates/network/src/tls/tests.rs (1)

1-41: LGTM!

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

58-58: LGTM!

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

277-284: LGTM!

Also applies to: 616-626, 687-694

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

5-6: LGTM!

Also applies to: 40-40, 65-66

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

475-627: LGTM!

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

18-20: LGTM!

Also applies to: 160-161, 171-171, 178-182, 194-195, 204-205, 237-237, 273-273, 283-392

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

69-94: LGTM!

Also applies to: 185-203, 213-264, 381-403

codecov · 2026-05-13T20:30:45Z

Codecov Report

❌ Patch coverage is 98.75000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 88.95%. Comparing base (996eb08) to head (cde2ff5).
⚠️ Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/network/src/lib.rs	96.77%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #490      +/-   ##
==========================================
- Coverage   90.30%   88.95%   -1.36%     
==========================================
  Files         121      121              
  Lines       11366    12700    +1334     
==========================================
+ Hits        10264    11297    +1033     
- Misses       1102     1403     +301

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

Copilot

Pull request overview

Ports pnpm v11-style TLS and local-address support into pacquet by extending .npmrc parsing, carrying the resolved settings through Config, and wiring them into the install-time reqwest client builder in pacquet-network.

Changes:

Add TlsConfig/TlsError to pacquet-network and apply CA roots, optional client identity (PKCS#8), strict-ssl, and local-address when building ThrottledClient::for_installs.
Extend .npmrc parsing in pacquet-config (ca, cafile, cert, key, strict-ssl, local-address) and apply results onto Config.tls.
Thread the unified ForInstallsError through the CLI init path and expand tests/fixtures to cover TLS parsing/build behavior.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
crates/package-manager/src/install_package_from_registry/tests.rs	Updates test config construction to include the new `tls` field.
crates/network/tests/fixtures/test-ca.pem	Adds shared PEM fixture for TLS tests.
crates/network/src/tls/tests.rs	Adds unit tests for `TlsConfig` defaults/clone and `TlsError` display.
crates/network/src/tls.rs	Introduces `TlsConfig` and `TlsError` types and associated docs.
crates/network/src/tests.rs	Updates proxy tests to pass TLS config and adds TLS/local-address build-time tests.
crates/network/src/lib.rs	Wires TLS into `ThrottledClient::for_installs`, adds `apply_tls`, and introduces `ForInstallsError`.
crates/config/src/npmrc_auth/tests.rs	Adds `.npmrc` parsing/apply tests for TLS + `local-address` keys (incl. `cafile` splitting).
crates/config/src/npmrc_auth.rs	Extends `NpmrcAuth` parsing/apply to populate `Config.tls` (and adds `cafile` loader + strict bool parser).
crates/config/src/lib.rs	Adds `Config::tls` and documents `.npmrc` key coverage and layering.
crates/cli/src/state.rs	Switches init to pass `&config.tls` into `for_installs` and updates error type to `ForInstallsError`.

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

Two CodeRabbit nitpicks on #490: - `crates/config/src/npmrc_auth.rs`: the `strict-ssl=` parse arm only wrote `auth.strict_ssl` on a successful `parse_bool`, so an `.npmrc` containing `strict-ssl=false` followed by a typo like `strict-ssl=oops` would leave the earlier `false` silently active — TLS verification disabled with no signal to the user. The arm now assigns `parse_bool(&value)` unconditionally so an invalid token resets the slot to `None` and the build-site `unwrap_or(true)` default kicks in. A regression test pins the multi-line ordering. - `crates/network/src/tls.rs`: the doc comment on `TlsError::InvalidClientIdentity` referenced `Identity::from_pem` (the rustls-only API) when the call site uses `Identity::from_pkcs8_pem` (the native-tls backend pacquet builds with). Updated to match the actual API, and added a pointer to the `apply_tls` doc that explains the PKCS#8 / PKCS#1 limitation.

Three doc-only nits flagged by Copilot on #490: - `TlsConfig.local_address` doc claimed the value "can be diagnosed early" but the config layer currently parses-and-silently-drops on invalid input (matching pnpm's parity policy of letting Node error at connect time). Updated the doc to describe the actual silent-drop semantics and note that a parse-time warning is a potential future enhancement. - `TEST_CA_PEM` fixture doc said discarding the private key keeps the cert "deterministic", but `openssl req -x509 -newkey rsa:2048` produces a fresh keypair / serial on every run. Reworded to describe the actual property (avoid committing key material; tests don't pin fingerprint or issuer). - `for_installs_strict_ssl_false_relaxes_verification` referenced a nonexistent `mockito_integration_strict_ssl_false_accepts_self_signed` "live-traffic counterpart". Replaced the dangling reference with an explanation that mockito only speaks plain HTTP, so a TLS-mock integration test would need a different harness (e.g. wiremock with rustls) — left as a future enhancement instead of citing a test that doesn't exist. A fourth Copilot comment (re-flagging the `Identity::from_pem` doc typo in `TlsError::InvalidClientIdentity`) is a duplicate of the CodeRabbit nit already fixed in a90ee38; the review just happened to be generated against the pre-fix commit.

coderabbitai

🧹 Nitpick comments (1)

crates/config/src/npmrc_auth/tests.rs (1)
538-638: ⚡ Quick win

Add diagnostic context for new assert! checks.

At Line 543, Line 597, Line 598, Line 610, and the default checks around Line 633-637, failures won’t show enough context. Please add assertion messages or nearby eprintln! values so CI failures are actionable without reruns.

Based on learnings: In Rust test code, for assertions like assert!/assert_ne!, print relevant actual/expected context when failures occur.
🤖 Prompt for 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.

In `@crates/config/src/npmrc_auth/tests.rs` around lines 538 - 638, The new
assertions in the tests need helpful failure context: update the assertions in
applies_inline_ca_to_config, cafile_reads_and_splits_into_per_cert_pems,
cafile_not_found_is_silently_treated_as_unset, inline_ca_and_cafile_concatenate,
and defaults_leave_tls_config_empty to include diagnostic messages or debug
values (e.g. use assert_eq!(..., ..., "unexpected tls.ca: {:?}", config.tls.ca)
or assert!(..., "tls.ca[0] not PEM: {:?}", config.tls.ca.get(0)), and/or add
small eprintln! lines printing key variables such as config.tls, config.tls.ca,
config.tls.cert, config.tls.key, and config.tls.local_address before the
assertions) so CI failures show actual vs expected state; locate these changes
around the test functions named above and update each assert/assert_eq/assert!
to include the contextual message or printed debug output.

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

Nitpick comments:
In `@crates/config/src/npmrc_auth/tests.rs`:
- Around line 538-638: The new assertions in the tests need helpful failure
context: update the assertions in applies_inline_ca_to_config,
cafile_reads_and_splits_into_per_cert_pems,
cafile_not_found_is_silently_treated_as_unset, inline_ca_and_cafile_concatenate,
and defaults_leave_tls_config_empty to include diagnostic messages or debug
values (e.g. use assert_eq!(..., ..., "unexpected tls.ca: {:?}", config.tls.ca)
or assert!(..., "tls.ca[0] not PEM: {:?}", config.tls.ca.get(0)), and/or add
small eprintln! lines printing key variables such as config.tls, config.tls.ca,
config.tls.cert, config.tls.key, and config.tls.local_address before the
assertions) so CI failures show actual vs expected state; locate these changes
around the test functions named above and update each assert/assert_eq/assert!
to include the contextual message or printed debug output.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 9c3fdbc0-9d13-48f0-aed7-d8d190432b88

📥 Commits

Reviewing files that changed from the base of the PR and between 1759f9d and a90ee38.

📒 Files selected for processing (3)

crates/config/src/npmrc_auth.rs
crates/config/src/npmrc_auth/tests.rs
crates/network/src/tls.rs

🚧 Files skipped from review as they are similar to previous changes (2)

crates/network/src/tls.rs
crates/config/src/npmrc_auth.rs

📜 Review details

🧰 Additional context used

📓 Path-based instructions (1)

**/*.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/config/src/npmrc_auth/tests.rs

🧠 Learnings (2)

📚 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/config/src/npmrc_auth/tests.rs

📚 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/config/src/npmrc_auth/tests.rs

🔇 Additional comments (1)

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

477-535: LGTM!

Copilot

Pull request overview

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

github-actions · 2026-05-13T20:41:46Z

Micro-Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.00     15.7±0.07ms   275.6 KB/sec    1.01     15.9±0.43ms   273.6 KB/sec

Two follow-up review comments on #490: - Copilot flagged that relative `cafile=certs/ca.pem` paths resolve against the *process* cwd, not the `.npmrc` directory — so a project `.npmrc` invoked via `pacquet --dir /project install` from `/home/user` reads `/home/user/certs/ca.pem` and silently drops the CA list. Verified that this matches pnpm exactly: upstream's `loadCAFile` passes the path straight to `fs.readFileSync` without `path.resolve`, and pnpm doesn't `process.chdir(opts.dir)` on `--dir` either. Per the cardinal rule (match pnpm even on surprising behavior), kept the current logic and added a long doc comment on `load_cafile` explaining the parity choice and the workarounds (absolute paths, `cd`-then-run, or the inline `ca=` form). A future PR could file an upstream issue and follow the fix into pacquet. - CodeRabbit nitpick: enriched `assert!` failure messages in `applies_inline_ca_to_config`, `cafile_reads_and_splits_into_per_cert_pems`, `inline_ca_and_cafile_concatenate`, and `defaults_leave_tls_config_empty` to print the relevant runtime state (the PEM string, the actual `tls.ca` / `tls.cert` / etc.) so a CI failure is actionable without rerunning the suite.

zkochan · 2026-05-13T20:47:55Z

@coderabbitai nitpick on crates/config/src/npmrc_auth/tests.rs (assert-context for the new TLS tests) — addressed in 0c73f27. Enriched the assert! failure messages in:

applies_inline_ca_to_config — now prints the actual PEM string when the BEGIN CERTIFICATE header check fails
cafile_reads_and_splits_into_per_cert_pems — both per-cert checks now include the split index and the PEM
inline_ca_and_cafile_concatenate — includes tls.ca in the assertion message
defaults_leave_tls_config_empty — each is_none / is_empty assertion prints the field's actual value

Failures now show actual vs. expected without needing a rerun.

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

coderabbitai · 2026-05-13T20:48:11Z

Tip

For best results, initiate chat on the files or code changes.

@zkochan Thanks for the detailed summary! The enriched failure messages across all four tests (applies_inline_ca_to_config, cafile_reads_and_splits_into_per_cert_pems, inline_ca_and_cafile_concatenate, and defaults_leave_tls_config_empty) look great — showing the actual value inline saves a debug cycle when a test fails. Appreciate the thorough follow-through on 0c73f27. ✅

Stable `cargo fmt --check` on CI [collapsed the multi-line form](https://github.com/pnpm/pacquet/actions/runs/25825509453/job/75877501198?pr=490) to a single line; local nightly fmt left it alone. Match CI's output so the format job goes green.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

coderabbitai

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/config/src/npmrc_auth.rs`:
- Around line 403-419: In load_cafile, don't drop a final malformed/truncated
chunk — change the filter logic that uses contents.split_inclusive(delimiter) so
it preserves any non-empty chunk instead of requiring
chunk.ends_with(delimiter); specifically, remove the ends_with(delimiter) guard
(or collect split_inclusive into a Vec and include every non-empty trimmed
chunk) so both well-formed certs and a final truncated chunk are returned,
keeping the original delimiter handling via the delimiter variable.

🪄 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: 235d63a8-d3f1-4375-b17c-91c1fdc1d571

📥 Commits

Reviewing files that changed from the base of the PR and between a90ee38 and c74f93b.

📒 Files selected for processing (4)

crates/config/src/npmrc_auth.rs
crates/config/src/npmrc_auth/tests.rs
crates/network/src/tests.rs
crates/network/src/tls.rs

🚧 Files skipped from review as they are similar to previous changes (3)

crates/network/src/tests.rs
crates/network/src/tls.rs
crates/config/src/npmrc_auth/tests.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). (8)

GitHub Check: Run benchmark on ubuntu-latest
GitHub Check: Run benchmark on ubuntu-latest
GitHub Check: Lint and Test (macos-latest)
GitHub Check: Lint and Test (windows-latest)
GitHub Check: Lint and Test (ubuntu-latest)
GitHub Check: Dylint
GitHub Check: copilot-pull-request-reviewer
GitHub Check: Code Coverage

🧰 Additional context used

📓 Path-based instructions (1)

**/*.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/config/src/npmrc_auth.rs

🧠 Learnings (1)

📚 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/config/src/npmrc_auth.rs

🔇 Additional comments (1)

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

185-265: LGTM!

Also applies to: 372-381

`load_cafile` previously used `split_inclusive(delimiter)` plus an `ends_with(delimiter)` guard, which silently dropped any non-empty chunk that followed the final `-----END CERTIFICATE-----` in a cafile. CodeRabbit caught the parity divergence on #490: pnpm's [`loadCAFile`](https://github.com/pnpm/pnpm/blob/94240bc046/config/reader/src/loadNpmrcFiles.ts#L251-L254) uses `split(delim)` + `${ca.trimStart()}${delim}`, which keeps the trailing chunk and re-appends the delimiter so downstream PEM parsing surfaces the truncation as `InvalidCa` rather than pacquet quietly leaving the user with a short CA list. Rewrote `load_cafile` to match pnpm byte-for-byte: - `split(delimiter)` instead of `split_inclusive` — pnpm drops the delimiter on the split side and re-appends it on the map side. - Filter on `chunk.trim().is_empty()` to drop the empty tail that appears when the file ends with a delimiter, while keeping non-empty malformed chunks. - `trim_start()` (not full `trim`) on the map side to match pnpm's `${ca.trimStart()}${delim}` whitespace handling. Added `cafile_trailing_garbage_is_preserved_for_downstream_parser` as a regression test pinning the new behavior: a bundle ending in `garbage-not-a-cert` (no closing delimiter) now produces 2 CA entries instead of 1, with the second entry carrying the delimiter appended onto the garbage so reqwest's `Certificate::from_pem` surfaces the failure.

github-actions · 2026-05-13T21:17:58Z

Integrated-Benchmark Report (Linux)

Scenario: Frozen Lockfile

Command	Mean [s]	Min [s]	Max [s]	Relative
`pacquet@HEAD`	2.526 ± 0.054	2.450	2.613	1.01 ± 0.04
`pacquet@main`	2.498 ± 0.072	2.408	2.637	1.00
`pnpm`	5.845 ± 0.097	5.706	6.000	2.34 ± 0.08

BENCHMARK_REPORT.json

{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 2.5264698141400004,
      "stddev": 0.053528301874624644,
      "median": 2.52458755634,
      "user": 2.61288552,
      "system": 3.53255858,
      "min": 2.4497662888400003,
      "max": 2.61288539984,
      "times": [
        2.50128924884,
        2.5411785808400005,
        2.4521488448400004,
        2.57562656884,
        2.61288539984,
        2.5171595168400005,
        2.5320155958400004,
        2.5806070888400003,
        2.4497662888400003,
        2.5020210078400003
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 2.4982491807400002,
      "stddev": 0.07249460202999176,
      "median": 2.4822414428400004,
      "user": 2.64108012,
      "system": 3.5237559799999993,
      "min": 2.40805061084,
      "max": 2.6365757478400003,
      "times": [
        2.40805061084,
        2.52348543684,
        2.6365757478400003,
        2.4840743318400005,
        2.4804085538400003,
        2.5977020168400005,
        2.43154434384,
        2.51502865684,
        2.46061818084,
        2.44500392784
      ]
    },
    {
      "command": "pnpm",
      "mean": 5.84485667914,
      "stddev": 0.09731701049684223,
      "median": 5.83913181434,
      "user": 8.51699192,
      "system": 4.245638679999999,
      "min": 5.70622723284,
      "max": 5.99957365984,
      "times": [
        5.75840433584,
        5.81765064284,
        5.70622723284,
        5.94418569984,
        5.8606129858400005,
        5.87290461084,
        5.7744514678400005,
        5.95135007984,
        5.76320607584,
        5.99957365984
      ]
    }
  ]
}

Scenario: Frozen Lockfile (Hot Cache)

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
`pacquet@HEAD`	746.0 ± 41.7	711.1	857.2	1.00
`pacquet@main`	825.4 ± 50.8	758.9	931.0	1.11 ± 0.09
`pnpm`	2405.1 ± 93.1	2329.2	2629.5	3.22 ± 0.22

BENCHMARK_REPORT.json

{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 0.74604867406,
      "stddev": 0.04173523251779526,
      "median": 0.7346548931600001,
      "user": 0.38751842000000003,
      "system": 1.60880778,
      "min": 0.7111316091600001,
      "max": 0.8571946301600001,
      "times": [
        0.8571946301600001,
        0.73646584416,
        0.73158454616,
        0.75225539716,
        0.72784512016,
        0.7111316091600001,
        0.71152831916,
        0.74543336216,
        0.73284394216,
        0.7542039701600001
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 0.82538596186,
      "stddev": 0.050757227558391,
      "median": 0.8219699501600001,
      "user": 0.37573512,
      "system": 1.6440495799999997,
      "min": 0.75888839616,
      "max": 0.93102133616,
      "times": [
        0.93102133616,
        0.76943690216,
        0.8208075691600001,
        0.8753492121600001,
        0.8299088211600001,
        0.75888839616,
        0.8229346041600001,
        0.84025155616,
        0.8210052961600001,
        0.7842559251600001
      ]
    },
    {
      "command": "pnpm",
      "mean": 2.4050888059599997,
      "stddev": 0.09305146961579137,
      "median": 2.3764637946600002,
      "user": 2.8339567200000007,
      "system": 2.1953203799999996,
      "min": 2.3291689021599997,
      "max": 2.62954100016,
      "times": [
        2.49970133416,
        2.4022975181599997,
        2.62954100016,
        2.3969255331599997,
        2.3291689021599997,
        2.37615788416,
        2.37137506416,
        2.37676970516,
        2.33217810816,
        2.33677301016
      ]
    }
  ]
}

zkochan · 2026-05-13T21:19:35Z

@coderabbitai review

coderabbitai · 2026-05-13T21:19:40Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Closes #497. ## Summary Adds per-registry TLS overrides keyed by nerf-darted `.npmrc` URI, the natural follow-up to #490's top-level TLS keys. Corporate environments running a private Verdaccio (or any registry with its own self-signed cert) can now pin scoped `:cafile=…` / `:cert=…` / `:key=…` per host without disabling strict-ssl globally. Three commits, layered: - **`feat(network)`** (eff1248): adds `RegistryTls` + `PerRegistryTls` types in `pacquet-network` plus the lookup machinery — `pick_for_url` ports pnpm's [5-step `pickSettingByUrl`](https://github.com/pnpm/pnpm/blob/94240bc046/network/fetch/src/dispatcher.ts#L338-L375) exactly (exact > nerf-dart > no-port > shorter prefix > recursive no-port retry). `ThrottledClient::for_installs` gains a third `&PerRegistryTls` parameter and pre-builds one reqwest `Client` per non-empty override. New `acquire_for_url(url: &str)` routes per-request; `acquire()` keeps the default-client behavior for callers without a URL. - **`feat(config)`** (4e69868): `NpmrcAuth` parses the six per-registry TLS suffixes (`:ca`, `:cafile`, `:cert`, `:certfile`, `:key`, `:keyfile`) matching pnpm's `SSL_SUFFIX_RE` and applies onto `Config.tls_by_uri`. `*file` variants read from disk at parse time (silent on error); inline values get `\\n` → `\n` expansion. `:cert` and `:certfile` share the same `tls.cert` slot — last-write-wins inside one `.npmrc`. - **`refactor(tarball,registry)`** (5f9cae9): three production call sites (registry metadata + version-tag fetches, plus two tarball download paths) move from `acquire()` to `acquire_for_url(url)` so the per-registry routing actually fires. ## Parity policy Bug-for-bug with pnpm v11 ([SHA 94240bc046](https://github.com/pnpm/pnpm/blob/94240bc046/config/reader/src/getNetworkConfigs.ts)): - **Field-by-field override**, not replace-all. Each scoped `ca` / `cert` / `key` overrides its top-level counterpart independently (mirroring upstream's `{ ...opts, ...sslConfig }` spread at `dispatcher.ts:143,264`). `strict_ssl` and `local_address` stay top-level-only — pnpm's regex doesn't recognize scoped versions. - **`ca` as `Option<String>`, not `Vec<String>`**: per-registry `ca` is a single string (possibly with concatenated `-----END CERTIFICATE-----` delimiters) — `reqwest::Certificate::from_pem` accepts both shapes. - **Inline `\\n` expansion only on per-registry**: pnpm applies `value.replace(/\\n/g, '\n')` to scoped values but not to top-level `ca=`. The divergence is intentional and matches upstream. - **Lax URI prefix check**: `foo:cert=…` (no `//` prefix) is accepted into the map with `uri_prefix = "foo"`. It never matches a real nerf-darted URL so the entry is dropped at lookup time, but storing it keeps byte-for-byte parsing parity with `tryParseSslKey`. ## Reviewer flags - **Per-registry clients duplicate connection pools.** Each unique override gets its own `reqwest::Client` and therefore its own connection pool. With N per-registry overrides the worker holds N+1 pools instead of one. The semaphore still bounds *concurrent in-flight requests* globally, but socket churn between registries with different TLS configs is now per-client. In practice most users have ≤2 overrides; if this becomes an issue we'd need to switch to rustls + custom certificate verifier (tracked under #499). - **`acquire_for_url` takes `&str` rather than `&Url`** so the existing `format!("{registry}{name}")` call sites don't need to round-trip through `Url::parse`. The lookup itself works on the raw string form via `nerf_dart`.

zkochan added 3 commits May 13, 2026 21:55

Copilot AI review requested due to automatic review settings May 13, 2026 20:25

Copilot started reviewing on behalf of zkochan May 13, 2026 20:25 View session

coderabbitai Bot suggested changes May 13, 2026

View reviewed changes

Comment thread crates/config/src/npmrc_auth.rs Outdated

Comment thread crates/network/src/tls.rs Outdated

Copilot AI reviewed May 13, 2026

View reviewed changes

Comment thread crates/network/src/tls.rs Outdated

Comment thread crates/network/src/tls.rs Outdated

Comment thread crates/network/src/tests.rs Outdated

Comment thread crates/network/src/tests.rs Outdated

zkochan added 2 commits May 13, 2026 22:31

Copilot AI review requested due to automatic review settings May 13, 2026 20:35

coderabbitai Bot reviewed May 13, 2026

View reviewed changes

coderabbitai Bot approved these changes May 13, 2026

View reviewed changes

Copilot started reviewing on behalf of zkochan May 13, 2026 20:35 View session

Copilot AI reviewed May 13, 2026

View reviewed changes

Comment thread crates/config/src/npmrc_auth.rs

Copilot AI review requested due to automatic review settings May 13, 2026 20:52

Copilot started reviewing on behalf of zkochan May 13, 2026 20:52 View session

Copilot AI reviewed May 13, 2026

coderabbitai Bot suggested changes May 13, 2026

View reviewed changes

Comment thread crates/config/src/npmrc_auth.rs

coderabbitai Bot approved these changes May 13, 2026

View reviewed changes

zkochan merged commit 55083a4 into main May 13, 2026
14 of 16 checks passed

zkochan deleted the feat/tls-keys-local-address branch May 13, 2026 21:20

zkochan mentioned this pull request May 13, 2026

feat(network): switch TLS backend to rustls for PKCS#1 + EC key support #509

Merged

9 tasks

Conversation

zkochan commented May 13, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Parity policy

Reviewer flags

Test plan

Upstream coverage

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review failed

Walkthrough

Changes

Sequence Diagram

Estimated code review effort

Possibly related PRs

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

github-actions Bot commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Micro-Benchmark Results

Linux

Uh oh!

zkochan commented May 13, 2026

Uh oh!

coderabbitai Bot commented May 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented May 13, 2026

Integrated-Benchmark Report (Linux)

Scenario: Frozen Lockfile

Scenario: Frozen Lockfile (Hot Cache)

Uh oh!

zkochan commented May 13, 2026

Uh oh!

coderabbitai Bot commented May 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zkochan commented May 13, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 13, 2026 •

edited

Loading

codecov Bot commented May 13, 2026 •

edited

Loading

github-actions Bot commented May 13, 2026 •

edited

Loading