refactor(config): rename pacquet-npmrc → pacquet-config and drop ini wiring#420
Conversation
Pnpm v11 reads project-structural settings from `pnpm-workspace.yaml` (camelCase) and limits `.npmrc` to auth/registry/network keys, so naming the runtime config crate after `.npmrc` no longer reflects what it does. This rename is pure: the crate moves to `crates/config/`, the `Npmrc` type becomes `Config`, and `pacquet-npmrc` / `pacquet_npmrc` references update to `pacquet-config` / `pacquet_config` workspace-wide. The `NpmrcAuth` helper that still parses the `.npmrc` auth subset keeps its name. The ini-based unit tests in `lib.rs` and the `serde_ini` deserializer attributes on `Config` survive untouched in this commit; both go in a follow-up cleanup. --- Written by an agent (Claude Code, claude-opus-4-7).
`Config` was carrying a full `Deserialize` impl plus per-field `serde_ini`-style `deserialize_with = "..."` attributes, even though pacquet no longer deserializes it from an ini file: yaml is parsed into `WorkspaceSettings` and applied onto a `Config` field-by-field, while `.npmrc` is parsed by the hand-rolled `NpmrcAuth::from_ini`. The serde machinery on `Config` was dead weight. Drops: - `Deserialize` + `#[serde(rename_all = "kebab-case")]` on `Config`, and every per-field `#[serde(default = "...", deserialize_with = "...")]`. - The deserializer helpers (`bool_true`, `deserialize_bool`, `deserialize_u32`, `deserialize_u64`, `deserialize_pathbuf`, `deserialize_store_dir`, `deserialize_registry`) — all of them were only referenced by the dropped attributes. - The `serde_ini` dependency (no longer used anywhere in the workspace). - The seven ini-based tests in `lib.rs` that constructed `Config` via `serde_ini::from_str(...)`; equivalent coverage already exists in `workspace_yaml/tests.rs` (camelCase mapping, registry trailing-slash normalisation, relative/absolute path resolution) and `npmrc_auth/tests.rs` (registry parsing from `.npmrc`). Renames the `custom_deserializer` module to `defaults` to reflect its remaining role: default-factory functions for `SmartDefault` (the `default_*()` entries used by `#[default(_code = "...")]`). Local variable `npmrc: &mut Config` in `apply_to` and `Config::current` becomes `config` to match the type, and the matching test fixtures rename in step. --- Written by an agent (Claude Code, claude-opus-4-7).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplace pacquet-npmrc/Npmrc with pacquet-config/Config, refactor the config crate to use code-driven defaults and a runtime Config type, and update CLI, package-manager, tests, and docs to the new crate and types. ChangesNpmrc → Config type migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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.
Pull request overview
This PR refactors pacquet’s runtime configuration layer to match pnpm 11’s split: project-structural settings come from pnpm-workspace.yaml, while .npmrc is limited to auth/registry/network. It renames the former pacquet-npmrc crate/type to pacquet-config/Config and removes now-dead serde_ini-based deserialization wiring and duplicate ini-centric tests.
Changes:
- Renamed
pacquet-npmrc→pacquet-configandNpmrc→Config, updating imports and dependencies across the workspace. - Removed
serde_inideserializer machinery from the runtime config struct, keeping.npmrcparsing as a small hand-rolled auth subset (NpmrcAuth). - Added/adjusted tests for YAML overlays and
.npmrcauth behavior; introduced a test env-var guard to reduce flakiness from env mutation under parallel test execution.
Reviewed changes
Copilot reviewed 34 out of 38 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tasks/integrated-benchmark/src/workspace_manifest.rs | Updates doc references from pacquet_npmrc to pacquet_config. |
| crates/tarball/src/lib.rs | Updates documentation to refer to the new Config boundary. |
| crates/reporter/src/tests.rs | Updates config enum path reference to pacquet_config. |
| crates/reporter/src/lib.rs | Updates doc reference to pacquet_config::PackageImportMethod. |
| crates/package-manager/src/symlink_direct_dependencies/tests.rs | Switches tests from Npmrc::new() to Config::new(). |
| crates/package-manager/src/symlink_direct_dependencies.rs | Updates struct field type from &'static Npmrc to &'static Config. |
| crates/package-manager/src/retry_config.rs | Renames input type and docs to Config. |
| crates/package-manager/src/link_file/tests.rs | Updates import path for PackageImportMethod. |
| crates/package-manager/src/link_file.rs | Updates import path for PackageImportMethod. |
| crates/package-manager/src/install/tests.rs | Switches tests from Npmrc::new() to Config::new(). |
| crates/package-manager/src/install.rs | Updates imports/types and doc references to Config. |
| crates/package-manager/src/install_without_lockfile.rs | Updates config reference type to Config. |
| crates/package-manager/src/install_package_from_registry/tests.rs | Updates helper/test config construction to Config. |
| crates/package-manager/src/install_package_from_registry.rs | Updates config reference type to Config. |
| crates/package-manager/src/install_package_by_snapshot.rs | Updates config reference type to Config. |
| crates/package-manager/src/install_frozen_lockfile.rs | Updates config reference type to Config. |
| crates/package-manager/src/create_virtual_store.rs | Updates config reference type to Config. |
| crates/package-manager/src/create_virtual_dir_by_snapshot/tests.rs | Updates import path for PackageImportMethod. |
| crates/package-manager/src/create_virtual_dir_by_snapshot.rs | Updates import path for PackageImportMethod. |
| crates/package-manager/src/create_cas_files.rs | Updates import path for PackageImportMethod. |
| crates/package-manager/src/add.rs | Updates config reference type to Config. |
| crates/package-manager/Cargo.toml | Swaps dependency pacquet-npmrc → pacquet-config. |
| crates/config/src/workspace_yaml/tests.rs | Updates tests to apply YAML settings onto Config. |
| crates/config/src/workspace_yaml.rs | Updates workspace-yaml overlay application to target Config. |
| crates/config/src/test_env_guard.rs | Adds a test-only env mutation guard (mutex + snapshot/restore). |
| crates/config/src/npmrc_auth/tests.rs | Updates tests to apply .npmrc auth subset onto Config (uses EnvGuard in one test). |
| crates/config/src/npmrc_auth.rs | Updates .npmrc auth subset parser to apply onto Config. |
| crates/config/src/lib.rs | Renames core type to Config and removes ini-deserialization attributes/helpers; updates tests accordingly. |
| crates/config/src/defaults/tests.rs | Adds tests for default_store_dir env behavior using EnvGuard. |
| crates/config/src/defaults.rs | Removes ini deserializer helpers, leaving only default factories. |
| crates/config/README.md | Adds crate README documenting settings coverage status. |
| crates/config/Cargo.toml | Renames crate to pacquet-config and drops serde_ini dependency. |
| crates/cli/src/state.rs | Updates state to hold &'static Config (doc comment still references .npmrc). |
| crates/cli/src/cli_args/store.rs | Updates store subcommand to accept Config provider. |
| crates/cli/src/cli_args.rs | Loads config via Config::current and updates types; closure name remains npmrc. |
| crates/cli/Cargo.toml | Swaps dependency pacquet-npmrc → pacquet-config. |
| Cargo.toml | Renames workspace member/dependency and removes serde_ini from workspace deps. |
| Cargo.lock | Updates lockfile for crate rename and removal of serde_ini (+ transitive 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 #420 +/- ##
=======================================
Coverage 85.23% 85.23%
=======================================
Files 79 79
Lines 5439 5372 -67
=======================================
- Hits 4636 4579 -57
+ Misses 803 793 -10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Micro-Benchmark ResultsLinux |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/config/src/lib.rs (1)
130-131:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing
#[default = true]onlockfilefield causes silent behavioral regression.The
lockfilefield defaults tofalse(SmartDefault's bool default), but pnpm enables lockfile generation by default. Other boolean fields that default totrue(e.g.,hoistat line 74,symlinkat line 111,prefer_frozen_lockfileat line 137) correctly use#[default = true].Without this fix, pacquet will silently skip lockfile generation/reading by default, diverging from pnpm.
🐛 Proposed fix
/// When set to false, pnpm won't read or generate a pnpm-lock.yaml file. + #[default = true] pub lockfile: bool,🤖 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/lib.rs` around lines 130 - 131, The bool field lockfile in the config struct is missing the SmartDefault attribute and thus defaults to false; add the attribute #[default = true] to the lockfile field (alongside the other boolean defaults like hoist, symlink, and prefer_frozen_lockfile) so that pub lockfile: bool defaults to true and pnpm-style lockfile generation/reading remains enabled by default.
🧹 Nitpick comments (1)
crates/config/src/workspace_yaml/tests.rs (1)
60-63: ⚡ Quick winAdd diagnostic logging around non-
assert_eq!assertions in these updated tests.
assert!/assert_ne!at Line 60, Line 62, Line 123, and Line 125 should log relevant values/context so failures are actionable without reruns.Based on learnings: "In Rust test code, follow the repo’s CODE_STYLE_GUIDE test-logging rule: add logging ... if you use assertions like `assert!`, `assert_ne!`, etc."Proposed patch
@@ settings.apply_to(&mut config, Path::new("/irrelevant-for-absolute-paths")); assert_eq!(config.store_dir, StoreDir::from(Path::new("/absolute/store").to_path_buf())); + eprintln!("lockfile after apply: {}", config.lockfile); assert!(!config.lockfile); assert_eq!(config.registry, "https://reg.example/"); + eprintln!( + "registry before apply: {before_registry}, after apply: {}", + config.registry + ); assert_ne!(before_registry, config.registry); @@ let mut config = Config::new(); + eprintln!( + "verify_store_integrity before apply: {}", + config.verify_store_integrity + ); assert!(config.verify_store_integrity, "the default is `true` to match pnpm"); settings.apply_to(&mut config, Path::new("/irrelevant")); + eprintln!( + "verify_store_integrity after apply: {}", + config.verify_store_integrity + ); assert!(!config.verify_store_integrity, "yaml override wins");Also applies to: 123-126
🤖 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/workspace_yaml/tests.rs` around lines 60 - 63, Tests use non-assert_eq! assertions without context; add diagnostic logging immediately before each assert!/assert_ne! to print relevant values so failures are actionable. For the assertions checking config.lockfile, config.registry and before_registry (and the similar assertions around lines 123–126), emit a clear diagnostic (e.g., eprintln!/tracing) showing the test name plus the values of config.lockfile, config.registry, before_registry and any other compared values before calling assert! or assert_ne!. Ensure logs are added in the same test function(s) that contain those assertions so failures show the captured state.
🤖 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.
Outside diff comments:
In `@crates/config/src/lib.rs`:
- Around line 130-131: The bool field lockfile in the config struct is missing
the SmartDefault attribute and thus defaults to false; add the attribute
#[default = true] to the lockfile field (alongside the other boolean defaults
like hoist, symlink, and prefer_frozen_lockfile) so that pub lockfile: bool
defaults to true and pnpm-style lockfile generation/reading remains enabled by
default.
---
Nitpick comments:
In `@crates/config/src/workspace_yaml/tests.rs`:
- Around line 60-63: Tests use non-assert_eq! assertions without context; add
diagnostic logging immediately before each assert!/assert_ne! to print relevant
values so failures are actionable. For the assertions checking config.lockfile,
config.registry and before_registry (and the similar assertions around lines
123–126), emit a clear diagnostic (e.g., eprintln!/tracing) showing the test
name plus the values of config.lockfile, config.registry, before_registry and
any other compared values before calling assert! or assert_ne!. Ensure logs are
added in the same test function(s) that contain those assertions so failures
show the captured state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b705ad4-9aa7-4c7d-aade-35346886554b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
Cargo.tomlcrates/cli/Cargo.tomlcrates/cli/src/cli_args.rscrates/cli/src/cli_args/store.rscrates/cli/src/state.rscrates/config/Cargo.tomlcrates/config/README.mdcrates/config/src/defaults.rscrates/config/src/defaults/tests.rscrates/config/src/lib.rscrates/config/src/npmrc_auth.rscrates/config/src/npmrc_auth/tests.rscrates/config/src/test_env_guard.rscrates/config/src/workspace_yaml.rscrates/config/src/workspace_yaml/tests.rscrates/package-manager/Cargo.tomlcrates/package-manager/src/add.rscrates/package-manager/src/create_cas_files.rscrates/package-manager/src/create_virtual_dir_by_snapshot.rscrates/package-manager/src/create_virtual_dir_by_snapshot/tests.rscrates/package-manager/src/create_virtual_store.rscrates/package-manager/src/install.rscrates/package-manager/src/install/tests.rscrates/package-manager/src/install_frozen_lockfile.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/package-manager/src/install_without_lockfile.rscrates/package-manager/src/link_file.rscrates/package-manager/src/link_file/tests.rscrates/package-manager/src/retry_config.rscrates/package-manager/src/symlink_direct_dependencies.rscrates/package-manager/src/symlink_direct_dependencies/tests.rscrates/reporter/src/lib.rscrates/reporter/src/tests.rscrates/tarball/src/lib.rstasks/integrated-benchmark/src/workspace_manifest.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Log emissions are part of 'match pnpm'—when porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(...), orstreamParser.write(...), mirror the call site, payload, and ordering so@pnpm/cli.default-reporterparses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing into plainStringor&strto preserve the distinction between validating and non-validating brands
If upstream always validates before construction of a branded string type, implement validation viaTryFrom<String>and/orFromStrwithout providing an infallible public constructor
If upstream never validates a branded string type, expose an infallibleFrom<String>constructor to provide type-safety without runtime validation
When upstream occasionally constructs a branded string type without validation via bareasassertion, expose afrom_str_uncheckedconstructor so callers can opt into the same unchecked path explicitly
Match upstream serde behavior for branded types—use#[serde(try_from = "String")]for deserialization to go through the validator and#[serde(into = "String")]for serialization
Use#[derive(derive_more::From)]and#[derive(derive_more::Into)]for mechanical conversion impls on branded types rather than handwritingimplblocks
Model string-literal unions (like'auto' | 'always' | 'never') as Rustenums instead of newtype wrappers
Template literal types (like${string}@${string}) should be treated as branded string types with the same validation discipline applied
Follow the code style guide inCODE_STYLE_GUIDE.mdcovering code-level conventions not enforced by tooling: imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching,pipe-trait, error handling, test layout, and cloning ofArcandRc
Warnings are errors incargo clippy --locked -- --deny warnings, and should not be silenced with `#[allow(....
Files:
crates/reporter/src/lib.rscrates/package-manager/src/link_file/tests.rscrates/reporter/src/tests.rscrates/package-manager/src/symlink_direct_dependencies.rscrates/package-manager/src/retry_config.rscrates/package-manager/src/install_package_from_registry.rstasks/integrated-benchmark/src/workspace_manifest.rscrates/cli/src/cli_args/store.rscrates/package-manager/src/install_frozen_lockfile.rscrates/package-manager/src/create_virtual_store.rscrates/package-manager/src/create_cas_files.rscrates/package-manager/src/symlink_direct_dependencies/tests.rscrates/package-manager/src/link_file.rscrates/config/src/workspace_yaml.rscrates/package-manager/src/install_without_lockfile.rscrates/config/src/npmrc_auth.rscrates/tarball/src/lib.rscrates/package-manager/src/install_package_by_snapshot.rscrates/package-manager/src/install_package_from_registry/tests.rscrates/package-manager/src/create_virtual_dir_by_snapshot.rscrates/config/src/npmrc_auth/tests.rscrates/cli/src/cli_args.rscrates/package-manager/src/add.rscrates/cli/src/state.rscrates/package-manager/src/install/tests.rscrates/package-manager/src/create_virtual_dir_by_snapshot/tests.rscrates/package-manager/src/install.rscrates/config/src/defaults.rscrates/config/src/workspace_yaml/tests.rscrates/config/src/lib.rs
🧠 Learnings (3)
📚 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/reporter/src/lib.rscrates/package-manager/src/link_file/tests.rscrates/reporter/src/tests.rscrates/package-manager/src/symlink_direct_dependencies.rscrates/package-manager/src/retry_config.rscrates/package-manager/src/install_package_from_registry.rstasks/integrated-benchmark/src/workspace_manifest.rscrates/cli/src/cli_args/store.rscrates/package-manager/src/install_frozen_lockfile.rscrates/package-manager/src/create_virtual_store.rscrates/package-manager/src/create_cas_files.rscrates/package-manager/src/symlink_direct_dependencies/tests.rscrates/package-manager/src/link_file.rscrates/config/src/workspace_yaml.rscrates/package-manager/src/install_without_lockfile.rscrates/config/src/npmrc_auth.rscrates/tarball/src/lib.rscrates/package-manager/src/install_package_by_snapshot.rscrates/package-manager/src/install_package_from_registry/tests.rscrates/package-manager/src/create_virtual_dir_by_snapshot.rscrates/config/src/npmrc_auth/tests.rscrates/cli/src/cli_args.rscrates/package-manager/src/add.rscrates/cli/src/state.rscrates/package-manager/src/install/tests.rscrates/package-manager/src/create_virtual_dir_by_snapshot/tests.rscrates/package-manager/src/install.rscrates/config/src/defaults.rscrates/config/src/workspace_yaml/tests.rscrates/config/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 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/link_file/tests.rscrates/reporter/src/tests.rscrates/package-manager/src/symlink_direct_dependencies/tests.rscrates/package-manager/src/install_package_from_registry/tests.rscrates/config/src/npmrc_auth/tests.rscrates/package-manager/src/install/tests.rscrates/package-manager/src/create_virtual_dir_by_snapshot/tests.rscrates/config/src/workspace_yaml/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/reporter/src/tests.rs
🔇 Additional comments (34)
crates/tarball/src/lib.rs (1)
105-106: Doc update correctly tracks the config rename.This wording change is consistent with the
Npmrc→Configrefactor and doesn’t alter runtime behavior.crates/config/Cargo.toml (1)
2-2: Crate rename is wired correctly in this manifest.
name = "pacquet-config"aligns with the workspace-wide migration.tasks/integrated-benchmark/src/workspace_manifest.rs (1)
2-3: Benchmark docs now correctly reference the new config flow.The comments are aligned with
WorkspaceSettingsapplying ontoConfig.Also applies to: 6-6
Cargo.toml (1)
27-27: Workspace dependency rename looks correct.
pacquet-configis declared at the workspace level as expected for downstream crates.crates/package-manager/Cargo.toml (1)
20-20: Dependency migration in package-manager is consistent.Using
pacquet-confighere matches the refactor across the codebase.crates/config/src/defaults.rs (1)
2-2: Import cleanup is fine.This is a non-functional simplification and stays consistent with the defaults module role.
crates/reporter/src/tests.rs (1)
98-99: Reporter test documentation update is correct.The reference now points to the right config crate/type source.
crates/package-manager/src/link_file/tests.rs (1)
5-5: Test import migration is correct.
PackageImportMethodnow comes frompacquet_config, matching the refactor.crates/reporter/src/lib.rs (1)
211-211: Doc reference rename looks correct.
Line 211 correctly referencespacquet_config::PackageImportMethod, consistent with the config crate migration.crates/cli/Cargo.toml (1)
22-22: Dependency rename is wired correctly.
Line 22 addspacquet-configin the expected workspace dependency slot for the CLI crate.crates/package-manager/src/create_cas_files.rs (1)
4-4: Import migration is consistent.
Line 4 correctly usespacquet_config::PackageImportMethodand keeps this module aligned with the new config crate.crates/package-manager/src/create_virtual_dir_by_snapshot/tests.rs (1)
2-2: Test import update is good.
Line 2 now points topacquet_config::PackageImportMethod, matching the package-manager migration.crates/cli/src/cli_args.rs (1)
91-93: Config loading migration is correctly applied.
Lines 91–93 switch the CLI loader toConfig::current(...).map(Config::leak), which aligns with the newConfig-based state flow.crates/package-manager/src/install_package_from_registry/tests.rs (1)
17-18: Test config migration is clean and consistent.
These lines correctly move test setup fromNpmrctoConfig, including the leaked static references used by the installer struct.Also applies to: 51-51, 130-130
crates/package-manager/src/symlink_direct_dependencies/tests.rs (1)
37-37:Config::new()migration in tests looks correct.
These updates consistently replace the old config type initialization without changing test intent.Also applies to: 157-157, 228-228
crates/package-manager/src/retry_config.rs (1)
1-1: Retry config wiring update is correct.
Theretry_opts_from_configinput type now usespacquet_config::Configconsistently with the rest of the refactor.Also applies to: 8-8
crates/package-manager/src/install_without_lockfile.rs (1)
11-11: Config type migration is consistent here.The
Npmrc→Configupdate is clean in the touched lines and doesn’t alter behavior in this module.Also applies to: 41-41
crates/package-manager/src/add.rs (1)
4-4: Looks good —Addnow cleanly depends onConfig.The updated import and field type are internally consistent with existing usage.
Also applies to: 23-23
crates/package-manager/src/create_virtual_store.rs (1)
7-7:CreateVirtualStoremigration is correct in the changed lines.The config crate/type rename is consistent and non-disruptive here.
Also applies to: 41-41
crates/package-manager/src/install_package_from_registry.rs (1)
7-7: Config migration is clean in this installer.Both touched lines align with the workspace-wide
Npmrc→Configtransition.Also applies to: 37-37
crates/package-manager/src/symlink_direct_dependencies.rs (1)
5-5: No issues with the config-type rename in this file.The changed lines are consistent with current field usage and flow.
Also applies to: 25-25
crates/config/src/workspace_yaml.rs (1)
1-1:WorkspaceSettings::apply_torename toConfigpreserves semantics.The touched changes keep the same behavior (field application, path resolution, and registry normalization) while completing the type migration.
Also applies to: 124-160
crates/config/src/npmrc_auth/tests.rs (1)
2-2: Test migration toConfigis solid.These updated tests still verify the same behaviors after the config-type refactor.
Also applies to: 11-13, 18-20, 29-48
crates/package-manager/src/install/tests.rs (1)
2-2: Install test updates are consistent with the config refactor.
Config::new()setup is applied uniformly across the touched tests with no behavioral drift in the modified segments.Also applies to: 40-40, 92-92, 127-127, 175-175, 248-248, 322-322, 381-381, 448-448, 579-579
crates/package-manager/src/link_file.rs (1)
3-3: Config crate migration in this module looks correct.
PackageImportMethodnow comes frompacquet_config, and this file’s import/match usage remains consistent.crates/cli/src/state.rs (1)
3-3:Statenow consistently depends onConfigend-to-end.The import, struct field, and initializer parameter all line up with the refactor goal.
Also applies to: 19-20, 50-51
crates/package-manager/src/install.rs (1)
9-9: Install pipeline config type migration is internally consistent.
Config/NodeLinkerusage, docs, and helper signatures align correctly with the crate rename.Also applies to: 32-33, 238-239, 259-259
crates/package-manager/src/install_frozen_lockfile.rs (1)
8-8: Frozen-lockfile installer migration toConfiglooks correct.No inconsistencies in the changed import/field typing.
Also applies to: 30-30
crates/package-manager/src/install_package_by_snapshot.rs (1)
6-6:InstallPackageBySnapshotnow consistently usesConfig.The changed import and field type are aligned and preserve existing flow.
Also applies to: 25-25
crates/config/src/npmrc_auth.rs (1)
1-1:NpmrcAuth→Configapplication path is correctly updated.The
apply_tosignature and trailing-slash normalization remain consistent after the type rename.Also applies to: 54-59
crates/cli/src/cli_args/store.rs (1)
3-3: Store command config callback migration looks good.
runnow correctly accepts aConfigprovider and existing call sites remain type-consistent.Also applies to: 24-27
crates/package-manager/src/create_virtual_dir_by_snapshot.rs (1)
4-4: LGTM!The import correctly reflects the crate rename from
pacquet-npmrctopacquet-config. Usage throughout the file remains consistent.crates/config/src/lib.rs (2)
61-78: LGTM on theConfigtype design and layering logic.The refactoring cleanly separates concerns:
SmartDefaultprovides compile-time defaultsNpmrcAuth::apply_tohandles.npmrcauth/registryWorkspaceSettings::apply_tooverlays YAML settingsThe layering order (defaults → npmrc → yaml) correctly matches pnpm 11's precedence.
Also applies to: 215-278
288-503: Test coverage is thorough.Tests properly validate:
- Default values match pnpm's expectations
.npmrcregistry application and non-auth key filteringpnpm-workspace.yamloverrides.npmrcsettings- Error propagation for invalid YAML
- Workspace manifest discovery by walking up directories
…ment Two follow-ups for #420: - `crates/cli/src/cli_args.rs`: the local closure that builds the runtime config was still named `npmrc`, even though it now returns a `Config` that includes `pnpm-workspace.yaml` overlays. Renamed to `config` and updated the call site in `CliCommand::Store`. - `crates/cli/src/state.rs`: `State::config`'s doc said "Configuration read from `.npmrc`", which no longer reflects the actual sources. Updated to describe the merged result (defaults + `.npmrc` auth subset + `pnpm-workspace.yaml`). --- Written by an agent (Claude Code, claude-opus-4-7).
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/src/cli_args.rs`:
- Around line 91-93: The config resolution is using env::current_dir inside the
closure passed to Config::current which ignores the CLI -C/--dir override;
update the closure so Config::current receives the canonicalized CLI directory
(the same dir/manifest_path you compute earlier) instead of env::current_dir,
ensuring the leaked Config::leak is created from that provided directory; apply
the same change where State::init and Store load config so all config resolution
uses the CLI-provided canonicalized dir rather than the process cwd.
🪄 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: 6dbc81e9-94cf-4942-9057-4fe533555d92
📒 Files selected for processing (2)
crates/cli/src/cli_args.rscrates/cli/src/state.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/cli/src/state.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). (4)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Code Coverage
- GitHub Check: Run benchmark on ubuntu-latest
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Log emissions are part of 'match pnpm'—when porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(...), orstreamParser.write(...), mirror the call site, payload, and ordering so@pnpm/cli.default-reporterparses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing into plainStringor&strto preserve the distinction between validating and non-validating brands
If upstream always validates before construction of a branded string type, implement validation viaTryFrom<String>and/orFromStrwithout providing an infallible public constructor
If upstream never validates a branded string type, expose an infallibleFrom<String>constructor to provide type-safety without runtime validation
When upstream occasionally constructs a branded string type without validation via bareasassertion, expose afrom_str_uncheckedconstructor so callers can opt into the same unchecked path explicitly
Match upstream serde behavior for branded types—use#[serde(try_from = "String")]for deserialization to go through the validator and#[serde(into = "String")]for serialization
Use#[derive(derive_more::From)]and#[derive(derive_more::Into)]for mechanical conversion impls on branded types rather than handwritingimplblocks
Model string-literal unions (like'auto' | 'always' | 'never') as Rustenums instead of newtype wrappers
Template literal types (like${string}@${string}) should be treated as branded string types with the same validation discipline applied
Follow the code style guide inCODE_STYLE_GUIDE.mdcovering code-level conventions not enforced by tooling: imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching,pipe-trait, error handling, test layout, and cloning ofArcandRc
Warnings are errors incargo clippy --locked -- --deny warnings, and should not be silenced with `#[allow(....
Files:
crates/cli/src/cli_args.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/cli/src/cli_args.rs
🔇 Additional comments (1)
crates/cli/src/cli_args.rs (1)
11-11: Config type migration here looks clean.Import rename to
pacquet_config::Configis consistent with the refactor intent.
`Config::current` was called with `env::current_dir`, which made `.npmrc` / `pnpm-workspace.yaml` resolution ignore the `-C/--dir` argument. A user running `pacquet --dir /elsewhere install` from a different cwd would load config from the cwd instead of `/elsewhere` — diverging from pnpm 11, whose [`loadNpmrcConfig`](https://github.com/pnpm/pnpm/blob/1819226b51/config/reader/src/loadNpmrcFiles.ts#L48-L50) builds `localPrefix` from `cliOptions.dir` (falling back to `cwd` only when `--dir` is unset). Pass the canonicalized `dir` (already computed above for the bunyan `prefix`) as the resolution root instead. With `--dir` defaulting to `.`, behaviour at the default call site is unchanged. Addresses the CodeRabbit review on PR #420. --- Written by an agent (Claude Code, claude-opus-4-7).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 38 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
crates/config/src/lib.rs:336
- The unsafe
env::set_var/env::remove_varcalls require that no other thread reads environment variables concurrently.EnvGuardserializes env-mutating tests, but other tests in this same test binary (e.g.have_default_values/fetch_retries_defaults_match_pnpm) callConfig::new()which readsPNPM_HOME/XDG_DATA_HOMEviadefault_store_dir()without taking the mutex. That violates the safety justification and can still cause UB/flakiness under parallel test execution. Consider taking the same lock in any test that callsConfig::new()/default_store_dir()(even read-only), or providing a helper that wraps those calls under the lock.
The `install_optional_failing_postinstall_dep_via_registry_mock_succeeds` test came in via the merge of `main` (commit 98c9886, PR #419 landed after this branch forked) and still constructed the config with `Npmrc::new()`. The rest of `pacquet-config` is `Config` now, so CI was failing to compile `pacquet-package-manager` (lib test) on both windows-latest and macos-latest. --- Written by an agent (Claude Code, claude-opus-4-7).
`SmartDefault` computes `modules_dir` / `virtual_store_dir` via [`defaults::default_modules_dir`] and [`defaults::default_virtual_store_dir`], both anchored at `env::current_dir()`. When `Config::current`'s caller passes a closure returning a different cwd — which is exactly what the CLI does after f66c3e3 to honour `-C/--dir` — those path defaults diverge from the resolution root: pacquet would load `.npmrc` / `pnpm-workspace.yaml` from `<--dir>` while still installing to `process_cwd/node_modules`. Re-anchor `modules_dir` / `virtual_store_dir` inside `Config::current` once `cwd` is known, before the `.npmrc` and yaml layers run. Yaml overrides still win on top. Matches pnpm 11, whose `modulesDir` / `virtualStoreDir` defaults are resolved against `pnpmConfig.dir`. Also drops the `cfg!(windows)` short-circuit from the `store_path_should_return_store_dir_from_pnpm_workspace_yaml` test's local `canonicalize` helper. The CLI canonicalizes `--dir` via `dunce::canonicalize` (added earlier for ndjson `prefix` correctness), and on Windows that resolves the GHA temp path `C:\Users\RUNNER~1\...` (short DOS form, what `TEMP` resolves to) into `C:\Users\runneradmin\...` (long form). The expected value has to canonicalize the same way to match. Addresses CodeRabbit's new review comment on PR #420 (the bug it flagged) and the windows-latest CI failure on `805a4124` (the canonicalization mismatch). The TODO the original test author left on the cfg-windows branch turned out to be precisely this issue. --- Written by an agent (Claude Code, claude-opus-4-7).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 39 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
crates/config/src/lib.rs:338
- These tests construct
Config::new()without holdingEnvGuard. Since other tests in this crate mutatePNPM_HOME/XDG_DATA_HOMEviaunsafe env::set_var/remove_var, reading env here can race with those writes under parallel test execution, violatingset_var’s safety contract. Either take the env mutex here (and in other non-mutating tests) or centralize env locking so allConfigconstruction in tests is serialized.
crates/config/src/workspace_yaml/tests.rs:56 - Several tests in this module call
Config::new()without holdingEnvGuard. Because other tests in the same crate mutate environment variables usingunsafe env::set_var/remove_var, these reads can run concurrently under the default parallel test harness and violate the safety requirements ofset_var. Please either acquire the env lock in these tests (even if they don’t mutate env) or centralize locking so anyConfigcreation in tests is serialized.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/config/src/lib.rs (1)
130-131:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd
#[default = true]to thelockfilefield to match pnpm's default behavior.pnpm's documented default for
lockfileistrue— when not explicitly set, lockfile generation is enabled. Without#[default = true]on line 131,SmartDefaultfalls back tobool::default()=false, causing pacquet to skip lockfile generation by default. This diverges from pnpm and is a behavioral regression. The pattern in the codebase confirms other enabled-by-default bool fields (hoist,symlink,prefer_frozen_lockfile,auto_install_peers) all declare#[default = true].Fix
/// When set to false, pnpm won't read or generate a pnpm-lock.yaml file. + #[default = true] pub lockfile: bool,🤖 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/lib.rs` around lines 130 - 131, The lockfile bool field is missing a SmartDefault of true, so SmartDefault currently yields false and changes behavior; update the struct field `lockfile` (the bool with doc "When set to false, pnpm won't read or generate a pnpm-lock.yaml file.") to include `#[default = true]` so it matches pnpm and the existing pattern used by fields like `hoist`, `symlink`, `prefer_frozen_lockfile`, and `auto_install_peers`; ensure the attribute is applied where SmartDefault derives are used so default construction enables lockfile generation.
🧹 Nitpick comments (1)
crates/config/src/lib.rs (1)
319-329: ⚡ Quick winConsider adding
lockfiledefault assertion.The test validates several boolean defaults (
prefer_frozen_lockfile,symlink,hoist) but notlockfile. Adding an assertion would guard against unintentional changes to this default:assert!(value.hoist); + assert!(value.lockfile); // or assert!(!value.lockfile) if false is intentional assert_eq!(value.store_dir, default_store_dir());🤖 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/lib.rs` around lines 319 - 329, The test have_default_values currently checks many Config::new() defaults but omits the lockfile field; update the test to assert the default lockfile value (e.g., assert_eq!(value.lockfile, <expected_default>)) so changes to Config::new()'s lockfile default are caught—locate the have_default_values test and add an assertion referencing value.lockfile and the expected default constant or literal used elsewhere.
🤖 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.
Outside diff comments:
In `@crates/config/src/lib.rs`:
- Around line 130-131: The lockfile bool field is missing a SmartDefault of
true, so SmartDefault currently yields false and changes behavior; update the
struct field `lockfile` (the bool with doc "When set to false, pnpm won't read
or generate a pnpm-lock.yaml file.") to include `#[default = true]` so it
matches pnpm and the existing pattern used by fields like `hoist`, `symlink`,
`prefer_frozen_lockfile`, and `auto_install_peers`; ensure the attribute is
applied where SmartDefault derives are used so default construction enables
lockfile generation.
---
Nitpick comments:
In `@crates/config/src/lib.rs`:
- Around line 319-329: The test have_default_values currently checks many
Config::new() defaults but omits the lockfile field; update the test to assert
the default lockfile value (e.g., assert_eq!(value.lockfile,
<expected_default>)) so changes to Config::new()'s lockfile default are
caught—locate the have_default_values test and add an assertion referencing
value.lockfile and the expected default constant or literal used elsewhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 86b1322a-1916-4869-abc4-957596061b00
📒 Files selected for processing (2)
crates/cli/tests/store.rscrates/config/src/lib.rs
✅ Files skipped from review due to trivial changes (1)
- crates/cli/tests/store.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). (5)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Agent
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Code Coverage
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Log emissions are part of 'match pnpm'—when porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(...), orstreamParser.write(...), mirror the call site, payload, and ordering so@pnpm/cli.default-reporterparses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing into plainStringor&strto preserve the distinction between validating and non-validating brands
If upstream always validates before construction of a branded string type, implement validation viaTryFrom<String>and/orFromStrwithout providing an infallible public constructor
If upstream never validates a branded string type, expose an infallibleFrom<String>constructor to provide type-safety without runtime validation
When upstream occasionally constructs a branded string type without validation via bareasassertion, expose afrom_str_uncheckedconstructor so callers can opt into the same unchecked path explicitly
Match upstream serde behavior for branded types—use#[serde(try_from = "String")]for deserialization to go through the validator and#[serde(into = "String")]for serialization
Use#[derive(derive_more::From)]and#[derive(derive_more::Into)]for mechanical conversion impls on branded types rather than handwritingimplblocks
Model string-literal unions (like'auto' | 'always' | 'never') as Rustenums instead of newtype wrappers
Template literal types (like${string}@${string}) should be treated as branded string types with the same validation discipline applied
Follow the code style guide inCODE_STYLE_GUIDE.mdcovering code-level conventions not enforced by tooling: imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching,pipe-trait, error handling, test layout, and cloning ofArcandRc
Warnings are errors incargo clippy --locked -- --deny warnings, and should not be silenced with `#[allow(....
Files:
crates/config/src/lib.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/lib.rs
🔇 Additional comments (2)
crates/config/src/lib.rs (2)
252-266: LGTM!The re-anchoring logic correctly fixes the path divergence issue where
SmartDefaultwould anchor paths toenv::current_dir()instead of the caller-supplied cwd. This ensurespacquet --dir <path>loads config and installs to the same directory, matching pnpm 11's behavior.
61-70: LGTM!The doc comment clearly explains that
Configis the merged runtime struct that is never deserialized directly —WorkspaceSettingsdeserializes from yaml and applies ontoConfig, while.npmrcauth parsing is handled separately. This accurately reflects the new architecture.
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.3679508867999997,
"stddev": 0.08120598239451296,
"median": 2.3516229439,
"user": 2.53321562,
"system": 3.2958009399999995,
"min": 2.2935866379,
"max": 2.5554739699,
"times": [
2.5554739699,
2.4171904959,
2.3200017389,
2.3120872208999996,
2.4019256148999997,
2.3011544538999997,
2.3927961359,
2.3832441489,
2.2935866379,
2.3020484508999997
]
},
{
"command": "pacquet@main",
"mean": 2.3453568359999997,
"stddev": 0.0699248621752831,
"median": 2.3605228178999997,
"user": 2.5124662199999994,
"system": 3.2523577399999994,
"min": 2.2370028828999997,
"max": 2.4247121958999998,
"times": [
2.2370422219,
2.2370028828999997,
2.3572648589,
2.4088551998999996,
2.4137032999,
2.4247121958999998,
2.3637807768999997,
2.3938996478999996,
2.3082720719,
2.3090352038999997
]
},
{
"command": "pnpm",
"mean": 5.6916927432,
"stddev": 0.07491135268545968,
"median": 5.6967811373999995,
"user": 8.24965992,
"system": 4.2167483400000005,
"min": 5.5488638939,
"max": 5.8232011719,
"times": [
5.7455430799,
5.653483286899999,
5.5488638939,
5.8232011719,
5.6718538148999995,
5.7289619789,
5.6356455479,
5.6576595179,
5.721708459899999,
5.7300066799
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6777982825800001,
"stddev": 0.06146047938249721,
"median": 0.6657229789800001,
"user": 0.33469334,
"system": 1.47653962,
"min": 0.6319982029800001,
"max": 0.84482985598,
"times": [
0.84482985598,
0.6753116489800001,
0.6319982029800001,
0.66828471598,
0.66316124198,
0.67297764798,
0.68630777198,
0.6607924179800001,
0.6366389949800001,
0.6376803269800001
]
},
{
"command": "pacquet@main",
"mean": 0.6658853429800001,
"stddev": 0.03847750626231726,
"median": 0.64862670948,
"user": 0.34266114,
"system": 1.4779566199999996,
"min": 0.6227058269800001,
"max": 0.74776724398,
"times": [
0.74776724398,
0.6582345919800001,
0.6967822199800001,
0.6402156409800001,
0.64770205698,
0.6451770709800001,
0.6227058269800001,
0.7059994719800001,
0.6495513619800001,
0.64471794398
]
},
{
"command": "pnpm",
"mean": 2.43854062338,
"stddev": 0.13376497962144987,
"median": 2.41365693848,
"user": 2.76902504,
"system": 2.15871002,
"min": 2.28437114698,
"max": 2.72145330298,
"times": [
2.32344800898,
2.37218100198,
2.28437114698,
2.72145330298,
2.48213479698,
2.4759933999799997,
2.37640247598,
2.3226991399799997,
2.45091140098,
2.57581155898
]
}
]
} |
…base The rebase onto main pulled in #420's rename of `pacquet-npmrc` → `pacquet-config`, which dropped the ini-wiring attributes (`#[serde(default = "bool_true", deserialize_with = "deserialize_bool")]`) from every field. My original commit on the now-renamed file carried those attributes along; once the helpers are gone they fail to compile (`cannot find attribute serde in this scope`). Strip them so the field matches the post-rename style — only the `#[default = true]` from `smart-default` remains, which is what the rest of the struct uses.
Pull in 28 commits from upstream main, including the `pacquet-npmrc` → `pacquet-config` rename (#420) plus features: - supportedArchitectures + --cpu/--os/--libc (#456) - frozen-lockfile (#442, #443, #447, #450) - git-fetcher (#436 / #446, #451, #454) - side-effects cache (#421 / #422, #423, #424) - real-hoist + global-virtual-store (#432 / #438 / #444, #449, #452) - patchedDependencies + allow-builds (#425, #427, #428) - engine/platform installability (#434 / #439) Conflicts resolved: - `crates/npmrc/` files migrated under the renamed `crates/config/` directory; `Npmrc` → `Config` everywhere except `NpmrcAuth` (which keeps the `.npmrc`-domain name). - `Config::current` reads the env-var DI generic `Api: EnvVar` for ${VAR}-substitution in `.npmrc`. Production turbofish in `cli_args.rs` is `Config::current::<RealApi, _, _, _, _>(...)`. - Two-phase `NpmrcAuth::apply_*` retained so default-registry creds key at the yaml-resolved registry URL. - New `Config::auth_headers` field plumbed through `install_package_by_snapshot`'s `DownloadTarballToStore`. - Tests under `crates/config/src/workspace_yaml/tests.rs` pick up the new ParseYaml unit test added on this branch.
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.
Resolves <#336>. ## Summary Ports pnpm v11's auth flow so `pacquet install` can talk to private registries on the same footing as `pnpm install`. Pacquet now parses credentials out of `.npmrc`, expands `${VAR}` references against the process environment, and attaches the right `Authorization` header on every metadata fetch and tarball download — including tarballs hosted on a CDN host that differs from the registry host. Upstream reference: [`pnpm/pnpm@601317e7a3`](https://github.com/pnpm/pnpm/tree/601317e7a3). ## What's wired up * `_auth`, `_authToken`, `username`, and `_password` keys, both default-registry (bare) and per-registry (`//host[:port]/path/:_authToken=…` family), parsed in `crates/config` (formerly `crates/npmrc`, renamed by #420). * `${VAR}` and `${VAR:-default}` substitution with backslash-escape semantics, ported from [`@pnpm/config.env-replace`](https://github.com/pnpm/components/blob/9c2bd17/config/env-replace/env-replace.ts). Unresolvable placeholders surface a `tracing::warn!` and leave the value verbatim — same best-effort behaviour as pnpm's [`substituteEnv`](https://github.com/pnpm/pnpm/blob/601317e7a3/config/reader/src/loadNpmrcFiles.ts#L156-L162). * URL-keyed lookup (`AuthHeaders`) in `crates/network`, mirroring [`createGetAuthHeaderByURI`](https://github.com/pnpm/pnpm/blob/601317e7a3/network/auth-header/src/index.ts): nerf-darts the request URL, walks parent path prefixes from longest to host-only, falls back to a port-stripped lookup matching upstream's [`removePort`](https://github.com/pnpm/pnpm/blob/601317e7a3/network/auth-header/src/helpers/removePort.ts) (any port, not just protocol defaults), and prefers inline `user:password@` basic auth when present. * Default-registry creds key against the *resolved* registry. The two-phase `apply_registry_and_warn` / `build_auth_headers` split inside `Config::current` ensures `pnpm-workspace.yaml`'s `registry` override propagates to the bearer-token nerf-dart key. * The `Authorization` header is attached on `Package::fetch_from_registry`, `PackageVersion::fetch_from_registry`, and inside `DownloadTarballToStore`'s retry loop. ## Test porting checklist Boxes ticked in `plans/TEST_PORTING.md` for the upstream auth tests this PR ports: * `network/auth-header/test/getAuthHeadersFromConfig.test.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 file` precedence, redirect header-stripping, the `installing/deps-installer/test/install/auth.ts` integration tests) need features pacquet doesn't yet have — token helpers, the `auth.ini` layer, scoped registries, redirect handling — and stay open for follow-ups. ## Notes / scope * `always-auth` is intentionally not honoured. pnpm v11 doesn't either — the auth header is selected by URL match alone, and `always-auth` was deprecated upstream. * TLS / proxy / scoped `@scope:registry` keys remain unparsed; the parser silently ignores them, matching the pre-#336 behaviour. The `creds_by_uri` shape is ready to grow as those land. * The 401/403/404 fail-fast retry policy in `crates/tarball` was already in place from #259; this PR's auth header attaches before the retry loop, so the policy still applies unchanged. * Env-var injection follows the trait-per-capability DI pattern from [#339](#339): `pub trait EnvVar` + `pub struct RealApi` in `crates/config/src/api.rs`, threaded through `env_replace`, `NpmrcAuth::from_ini`, and `Config::current` under one `Api: EnvVar` bound. Production callers turbofish `RealApi` explicitly: `Config::current::<RealApi, _, _, _, _>(...)`.
Summary
pnpm-workspace.yamland limits.npmrcto auth/registry/network keys. The crate that holds pacquet's resolved runtime config was still namedpacquet-npmrcwith aNpmrctype and carriedserde_inideserializer wiring on every field, which no longer reflected how the config is actually loaded on this branch.Commits
refactor(config): rename pacquet-npmrc crate to pacquet-config— mechanical rename.crates/npmrc/→crates/config/,Npmrc→Config,pacquet-npmrc/pacquet_npmrcreferences updated workspace-wide.NpmrcAuth(the.npmrcauth-subset parser) keeps its name.refactor(config): drop ini deserializer wiring from Config— removesDeserialize+#[serde(rename_all = "kebab-case")]fromConfig, every per-fielddeserialize_with/default = "..."attribute, the helper fns that backed them (bool_true,deserialize_bool/u32/u64/pathbuf/store_dir/registry), and theserde_iniworkspace dep. Drops seven ini-basedlib.rstests that already had equivalent yaml/npmrc-auth coverage. Renamescustom_deserializer→defaults(only default factories left), and thenpmrc: &mut Configlocal inapply_to/Config::currentbecomesconfig.After the cleanup
Configis purely a merged runtime struct: yaml deserializes intoWorkspaceSettingsand applies onto aConfig,.npmrcis hand-parsed byNpmrcAuth::from_ini, andConfigitself is no longer deserialized from any file.Test plan
cargo check --locked(clean)just lint(no clippy warnings)just test— 464 passed / 2 skipped (delta from main = the seven removed ini-based duplicates)taplo format --checktyposWritten by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit