feat: proxy support (.npmrc + env-var cascade, HTTP/HTTPS/SOCKS)#476
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📜 Recent 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). (7)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (3)📚 Learning: 2026-05-07T23:19:08.272ZApplied to files:
📚 Learning: 2026-05-01T10:01:33.766ZApplied to files:
📚 Learning: 2026-05-01T10:01:33.766ZApplied to files:
🔇 Additional comments (7)
📝 WalkthroughWalkthroughAdds proxy configuration types and parsing, applies an npmrc→legacy→env cascade to populate Config.proxy, builds a proxy-aware ThrottledClient (including SOCKS support), implements no-proxy matching and userinfo decoding, updates CLI wiring/error variants, and adds unit/integration tests. ChangesProxy Support Implementation
Sequence Diagram(s)sequenceDiagram
participant NpmrcAuth
participant EnvLookup as Environment Lookup
participant Config
participant ProxyConfig
participant ThrottledClient
participant Parser as parse_proxy_url
participant Matcher as NoProxyMatcher
NpmrcAuth->>NpmrcAuth: parse .npmrc proxy keys
NpmrcAuth->>EnvLookup: consult env fallbacks (HTTPS_PROXY/HTTP_PROXY/PROXY)
NpmrcAuth->>ProxyConfig: apply_proxy_cascade -> resolved proxy values
ProxyConfig-->>Config: set Config.proxy
Config->>ThrottledClient: for_installs(&config.proxy)
ThrottledClient->>Parser: parse_proxy_url(https/http proxy)
Parser-->>ThrottledClient: parsed Url or ProxyError
ThrottledClient->>Matcher: build NoProxyMatcher from no_proxy
ThrottledClient->>ThrottledClient: default_client_builder + build_scheme_proxy (attach proxy closures)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/network/src/tests.rs (1)
29-66: ⚡ Quick winAdd failure-context logging around non-
assert_eq!assertions.These boolean assertions would be easier to diagnose under
nextestif they log the relevant runtime context (host, matcher config, or error value) before asserting.Based on learnings: In Rust test code, add logging for assertions like
assert!/assert_ne!so failures are diagnosable without reruns.Also applies to: 129-135, 154-160
🤖 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/network/src/tests.rs` around lines 29 - 66, The tests use bare assert! calls that lack failure context; update each assertion in functions like no_proxy_matcher_reverse_dot_match, no_proxy_matcher_empty_entries_never_match, no_proxy_matcher_multiple_entries, no_proxy_bypass_short_circuits_every_host, and no_proxy_none_matches_nothing to include diagnostic info by logging or embedding a message with the runtime values (e.g., the tested host, the NoProxyMatcher/NoProxySetting used, and any unexpected return from NoProxyMatcher::matches_host) so failures show host and matcher config; you can either call dbg!/eprintln! with the host and matcher before the assert or use assert!(..., "host={:?} matcher={:?} expected=... got=...") to provide actionable context.crates/network/src/lib.rs (1)
246-254: 💤 Low valueMinor documentation inaccuracy: JS
decodeURIComponentthrows on invalid sequences.The comment states this mirrors "
decodeURIComponent's lenient fallback," but JavaScript'sdecodeURIComponentactually throwsURIErroron malformed sequences like%ZZrather than keeping them verbatim. The current implementation (keeping invalid sequences) is actually safer and a reasonable choice—the comment could be clarified to say "unlikedecodeURIComponentwhich throws, invalid sequences are kept verbatim."🤖 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/network/src/lib.rs` around lines 246 - 254, The doc comment for percent_decode_str incorrectly claims it mirrors decodeURIComponent's "lossy behavior" and lenient fallback; update the comment on percent_decode_str to clarify that, unlike JavaScript's decodeURIComponent which throws a URIError on malformed %XX sequences, this function intentionally keeps invalid percent-escapes verbatim (and explain that this is a deliberate, safer behavior), while preserving the rest of the descriptive text about why the hand-rolled decoder exists.
🤖 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/network/src/lib.rs`:
- Around line 246-254: The doc comment for percent_decode_str incorrectly claims
it mirrors decodeURIComponent's "lossy behavior" and lenient fallback; update
the comment on percent_decode_str to clarify that, unlike JavaScript's
decodeURIComponent which throws a URIError on malformed %XX sequences, this
function intentionally keeps invalid percent-escapes verbatim (and explain that
this is a deliberate, safer behavior), while preserving the rest of the
descriptive text about why the hand-rolled decoder exists.
In `@crates/network/src/tests.rs`:
- Around line 29-66: The tests use bare assert! calls that lack failure context;
update each assertion in functions like no_proxy_matcher_reverse_dot_match,
no_proxy_matcher_empty_entries_never_match, no_proxy_matcher_multiple_entries,
no_proxy_bypass_short_circuits_every_host, and no_proxy_none_matches_nothing to
include diagnostic info by logging or embedding a message with the runtime
values (e.g., the tested host, the NoProxyMatcher/NoProxySetting used, and any
unexpected return from NoProxyMatcher::matches_host) so failures show host and
matcher config; you can either call dbg!/eprintln! with the host and matcher
before the assert or use assert!(..., "host={:?} matcher={:?} expected=...
got=...") to provide actionable context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f71e987e-418d-40ae-90ae-415f411ad32f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.tomlcrates/cli/src/state.rscrates/config/src/lib.rscrates/config/src/npmrc_auth.rscrates/config/src/npmrc_auth/tests.rscrates/network/Cargo.tomlcrates/network/src/lib.rscrates/network/src/tests.rscrates/package-manager/src/install_package_from_registry/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). (9)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Dylint
- GitHub Check: Code Coverage
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Cargo Deny
- GitHub Check: Doc
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/install_package_from_registry/tests.rscrates/cli/src/state.rscrates/network/src/tests.rscrates/config/src/npmrc_auth.rscrates/config/src/npmrc_auth/tests.rscrates/config/src/lib.rscrates/network/src/lib.rs
🧠 Learnings (3)
📚 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/network/src/tests.rscrates/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/package-manager/src/install_package_from_registry/tests.rscrates/cli/src/state.rscrates/network/src/tests.rscrates/config/src/npmrc_auth.rscrates/config/src/npmrc_auth/tests.rscrates/config/src/lib.rscrates/network/src/lib.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 (17)
Cargo.toml (1)
64-70: LGTM!crates/network/Cargo.toml (1)
14-23: LGTM!crates/package-manager/src/install_package_from_registry/tests.rs (1)
56-56: LGTM!crates/config/src/npmrc_auth/tests.rs (1)
6-223: LGTM!crates/cli/src/state.rs (1)
5-5: LGTM!Also applies to: 34-40, 62-67
crates/config/src/lib.rs (1)
102-145: LGTM!Also applies to: 309-315, 639-696, 1100-1143
crates/network/src/tests.rs (1)
1-27: LGTM!Also applies to: 68-127, 137-153, 162-258
crates/config/src/npmrc_auth.rs (1)
1-1: LGTM!Also applies to: 5-10, 19-31, 58-65, 72-117, 120-131
crates/network/src/lib.rs (9)
1-9: LGTM!
11-12: LGTM!
139-174: LGTM!
187-202: LGTM!
204-223: LGTM!
225-244: LGTM!
273-297: LGTM!
299-347: LGTM!
349-369: LGTM!
Micro-Benchmark ResultsLinux |
There was a problem hiding this comment.
Pull request overview
Adds end-to-end proxy support to pacquet (ported from pnpm v11), spanning config parsing/cascade (.npmrc + env), network client proxy routing (HTTP/HTTPS/SOCKS), and CLI state initialization with proxy-aware client construction.
Changes:
- Introduces
ProxyConfig+NoProxySettingand extends.npmrcparsing to read proxy keys and apply an upstream-matching env-var fallback cascade. - Wires proxy routing into
pacquet-networkviareqwest::Proxy::custom, includingnoProxymatching and proxy basic-auth handling, plus integration/unit tests. - Updates CLI initialization to build the install HTTP client using the resolved proxy config; enables reqwest’s
socksfeature.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 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 Config.proxy field. |
| crates/network/src/tests.rs | Adds unit + mockito integration tests covering proxy routing, auth decoding, no-proxy behavior, and invalid proxy URL diagnostics. |
| crates/network/src/lib.rs | Implements proxy-aware ThrottledClient::for_installs, noProxy matcher, proxy URL parsing, and ProxyError diagnostic. |
| crates/network/Cargo.toml | Adds direct deps needed for proxy config/diagnostics and dev-deps for mockito + tokio test runtime. |
| crates/config/src/npmrc_auth/tests.rs | Adds focused tests for .npmrc proxy key parsing and env cascade behavior without mutating process env. |
| crates/config/src/npmrc_auth.rs | Extends .npmrc parser and applies proxy/env cascade into Config.proxy; adds no-proxy parsing. |
| crates/config/src/lib.rs | Introduces ProxyConfig/NoProxySetting, threads proxy config through Config, and ensures cascade runs even without an .npmrc. |
| crates/cli/src/state.rs | Builds install HTTP client using ThrottledClient::for_installs(&config.proxy) and plumbs ProxyError into init errors. |
| Cargo.toml | Enables reqwest socks feature. |
| Cargo.lock | Locks new/updated dependency graph due to added features/deps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #476 +/- ##
==========================================
+ Coverage 89.61% 90.25% +0.64%
==========================================
Files 119 120 +1
Lines 11098 11274 +176
==========================================
+ Hits 9945 10175 +230
+ Misses 1153 1099 -54 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.50910181082,
"stddev": 0.0924958671135193,
"median": 2.49958406352,
"user": 2.59299372,
"system": 3.4954548999999995,
"min": 2.37169913052,
"max": 2.64039704552,
"times": [
2.50438981352,
2.62782867652,
2.59065002052,
2.49477831352,
2.39618989552,
2.48271599652,
2.64039704552,
2.43723775952,
2.54513145652,
2.37169913052
]
},
{
"command": "pacquet@main",
"mean": 2.51064699442,
"stddev": 0.10999635268098712,
"median": 2.4650736375199997,
"user": 2.6100618199999994,
"system": 3.4520275000000007,
"min": 2.41212761552,
"max": 2.75083732852,
"times": [
2.57885647552,
2.43013646052,
2.58285924752,
2.41212761552,
2.57820261452,
2.44893219652,
2.75083732852,
2.42744370552,
2.41585922152,
2.48121507852
]
},
{
"command": "pnpm",
"mean": 5.668226666920001,
"stddev": 0.05458097522551744,
"median": 5.68664131202,
"user": 8.206329019999998,
"system": 4.2088825,
"min": 5.57737465652,
"max": 5.72904537652,
"times": [
5.72904537652,
5.61614964352,
5.61089960452,
5.71387998552,
5.71311175952,
5.66328176452,
5.57737465652,
5.71000085952,
5.71246532552,
5.63605769352
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.72368352764,
"stddev": 0.030244646253980476,
"median": 0.71613424394,
"user": 0.36098489999999994,
"system": 1.61454634,
"min": 0.70292672994,
"max": 0.8076313649400001,
"times": [
0.8076313649400001,
0.71688721994,
0.71658193494,
0.71568655294,
0.7119927999400001,
0.72339702094,
0.71330568094,
0.7049893109400001,
0.70292672994,
0.72343666094
]
},
{
"command": "pacquet@main",
"mean": 0.8473845346400001,
"stddev": 0.08885313699775434,
"median": 0.80708801844,
"user": 0.3588743,
"system": 1.6289679399999997,
"min": 0.7542094019400001,
"max": 0.97461032794,
"times": [
0.96124400194,
0.8040948439400001,
0.97461032794,
0.8549959219400001,
0.77048862194,
0.78863505594,
0.97441436394,
0.78107161394,
0.7542094019400001,
0.81008119294
]
},
{
"command": "pnpm",
"mean": 2.40650303814,
"stddev": 0.13842249689950184,
"median": 2.34283956644,
"user": 2.7767676999999997,
"system": 2.14342684,
"min": 2.29105102294,
"max": 2.66074321894,
"times": [
2.55545016294,
2.31183661694,
2.66074321894,
2.35033023394,
2.33534889894,
2.29105102294,
2.36905562194,
2.29251762794,
2.31324534594,
2.58545163094
]
}
]
} |
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.
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()`).
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.
5aed74b to
b03ee45
Compare
Rebase + review-comment passForce-pushed onto the latest CodeRabbit nitpicks
Copilot inline comments
Architectural change from rebaseThe auth feature on CI parity sweep on the rebased branch: Written by an agent (Claude Code, claude-opus-4-7). |
Closes #482. ## Summary Ports pnpm v11's TLS + `local-address` `.npmrc` keys onto pacquet (SHA [`94240bc046`](https://github.com/pnpm/pnpm/blob/94240bc046/network/fetch/src/dispatcher.ts)), 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`](https://github.com/pnpm/pnpm/blob/94240bc046/config/reader/src/loadNpmrcFiles.ts#L249-L265). 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](#482) 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 #482 as a follow-up.
Summary
Adds proxy support to pacquet, ported from pnpm v11 (SHA 94240bc046).
feat(config)):Config.proxy: ProxyConfig+NoProxySettingenum.NpmrcAuthparseshttps-proxy,http-proxy,proxy(legacy),no-proxy, andnoproxyfrom.npmrc. The env-var fallback cascade (HTTPS_PROXY/HTTP_PROXY/PROXY/NO_PROXY, each with upper- and lowercase lookups) matches upstream'sconfig/reader/src/index.ts:591-600..npmrcalways wins over env;http_proxyfalls back through the resolvedhttps_proxybefore consulting env.feat(network)):ThrottledClient::for_installs(&ProxyConfig)wires per-URL routing viareqwest::Proxy::custom. The closure dispatches HTTPS targets throughhttps_proxy, HTTP throughhttp_proxy, with aNoProxyMatcher(reverse-dot-segment-prefix port of upstream'scheckNoProxy) short-circuiting both. Basic-auth userinfo is stripped from the URL and percent-decoded before being re-attached viaProxy::basic_auth, matching pnpm'sdecodeURIComponent(user):decodeURIComponent(pass). Invalid proxy URLs surface asProxyError::InvalidProxycarrying diagnostic codeERR_PNPM_INVALID_PROXY.feat(cli)):State::initswitches to the config-aware constructor;InitStateError::Proxypropagates parse failures.Reviewer flags
reqwest'ssocksCargo feature (in rootCargo.toml). It is a feature on an existing dep, not a new top-level dep, but it pulls a SOCKS implementation into the build. socks4/socks4a/socks5 proxy URLs are now honored.crates/network/Cargo.tomlgains direct deps onpacquet-config,derive_more,miette(all already in[workspace.dependencies]; no new top-level additions).parse_proxy_urlrejects any first-attempt parse that lacks a host, forcing thehttp://-prefix retry through the authority-bearing path. Rust'surl::Url::parse("proxy.example:8080")is permissive enough to accept it as scheme + opaque path, which is not what users mean by a proxy shorthand.percent-encodingdirect dep — the only call site is the two halves of a proxy URL.Test plan
cargo nextest run -p pacquet-config— 70 tests pass (11 new cascade tests + 1EnvGuardsmoke test)cargo nextest run -p pacquet-network— 19 tests pass (matcher, parse, percent-decode, build, plus 2 mockito integration tests for proxy forwarding with basic-auth and noProxy bypass)cargo nextest run --workspace— full 666-test suite passjust ready— typos, fmt, check, lint cleantaplo format --checkcleanRUSTDOCFLAGS=-D warnings cargo doc --no-deps --workspace --document-private-itemscleanjust dylint(perfectionist) cleanHTTPS_PROXY=http://localhost:9999 pacquet install, traffic routes to the proxyUpstream coverage
Ports the unit-testable cases from pnpm's
network/fetch/test/dispatcher.test.ts: per-URL routing, basic-auth decoding, noProxy reverse-dot-prefix match, bypass-all literal, invalid URL →ERR_PNPM_INVALID_PROXY, SOCKS-URL parse smoke. Skips upstream'sdescribe.skip-ed live-SOCKS test.Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
New Features
Improvements