Conversation
Pacquet's config loader only read `pnpm-workspace.yaml` (plus the auth/network subset of `.npmrc`). Two sources pnpm v11 reads were missing: - `<configDir>/config.yaml` (e.g. `~/.config/pnpm/config.yaml`, `~/Library/Preferences/pnpm/config.yaml`, `%LOCALAPPDATA%/pnpm/config/config.yaml`). Now loaded between `.npmrc` and `pnpm-workspace.yaml`, with workspace-only keys (`nodeLinker`, `hoist`, `lockfile`, …) filtered out to mirror upstream's `isConfigFileKey` allowlist at `index.ts:299-309`. - `PNPM_CONFIG_*` / `pnpm_config_*` env vars across the whole schema (pacquet previously only honored `NPM_CONFIG_WORKSPACE_DIR`). Now read for every `WorkspaceSettings` field and applied after `pnpm-workspace.yaml` so env vars win — matching upstream's `parseEnvVars` loop at `index.ts:471-488`. Closes the scenario in #11738: a user with `enableGlobalVirtualStore: true` in `~/.config/pnpm/config.yaml` running an install in a project whose `pnpm-workspace.yaml` doesn't repeat the setting now lands in the global virtual store, same as pnpm. Note: the original issue proposed reading the full setting surface from `.npmrc`, `~/.pnpmrc`, `~/.config/pnpm/rc`, and `npm_config_*` env vars. Those proposals are anti-parity — pnpm v11 reads none of them for non-auth/non-network settings (see `isNpmrcReadableKey` at `localConfig.ts:197-198` and the `pnpm_config_*`-only prefix filter in `env.ts:185-195`). Implementing them would diverge from pnpm. --- Written by an agent (Claude Code, claude-opus-4-7).
|
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 selected for processing (2)
🚧 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). (8)
📝 WalkthroughWalkthroughAdds platform-aware global config-dir resolution, global ChangesGlobal configuration and environment variable layering
Sequence Diagram(s)sequenceDiagram
participant Config as Config::current
participant Npmrc as .npmrc loader
participant GlobalYAML as WorkspaceSettings::load_global
participant WorkspaceYAML as pnpm-workspace.yaml
participant EnvVars as WorkspaceSettings::from_pnpm_config_env
participant Registry as auth header builder
Config->>Npmrc: load auth/network subset
Npmrc-->>Config: partial settings
Config->>GlobalYAML: load_global(config_dir)
GlobalYAML-->>Config: settings + explicit flags
Config->>WorkspaceYAML: load workspace YAML
WorkspaceYAML-->>Config: settings + accumulate explicit flags
Config->>EnvVars: from_pnpm_config_env()
EnvVars-->>Config: settings + accumulate explicit flags
Config->>Config: restore workspace_dir anchoring
Config->>Registry: rebuild auth headers after registry finalized
Registry-->>Config: final config
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Micro-Benchmark ResultsLinux |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pacquet/crates/config/src/workspace_yaml.rs (1)
275-286:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse file-agnostic error messages for shared YAML loader errors.
At Line 304+ (
load_global), parse/read failures forconfig.yamlare surfaced throughLoadWorkspaceYamlError, but the variant messages currently saypnpm-workspace.yaml. This makes global-config failures misleading for users and debugging.Suggested fix
pub enum LoadWorkspaceYamlError { - #[display("Failed to read pnpm-workspace.yaml at {}: {source}", path.display())] + #[display("Failed to read YAML config at {}: {source}", path.display())] ReadFile { path: PathBuf, #[error(source)] source: io::Error, }, - #[display("Failed to parse pnpm-workspace.yaml at {}: {source}", path.display())] + #[display("Failed to parse YAML config at {}: {source}", path.display())] ParseYaml { path: PathBuf, #[error(source)] source: Box<serde_saphyr::Error>, }, }Also applies to: 304-314
🤖 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 `@pacquet/crates/config/src/workspace_yaml.rs` around lines 275 - 286, The error variants ReadFile and ParseYaml on LoadWorkspaceYamlError hardcode "pnpm-workspace.yaml", causing misleading messages when load_global parses other files (e.g., config.yaml); update their display strings to be file-agnostic (e.g., "Failed to read workspace YAML at {}: {source}" and "Failed to parse workspace YAML at {}: {source}") so they use the provided path.display() generically, and apply the same change to any other occurrences of those variant messages referenced from load_global or similar loaders.
🤖 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 `@pacquet/crates/config/src/lib.rs`:
- Around line 1051-1060: The code currently unconditionally sets
self.virtual_store_dir = base_dir.join("node_modules/.pnpm") before applying
workspace settings, which can overwrite a global virtual_store_dir; change the
logic so you do not clobber a globally-configured value: remove the
unconditional assignment and instead only set self.virtual_store_dir to
base_dir.join("node_modules/.pnpm") when neither the workspace settings provide
virtual_store_dir (settings.virtual_store_dir.is_none()) nor a
previously-explicit global value exists (use virtual_store_dir_explicit /
global_virtual_store_dir_explicit to decide); keep updating
virtual_store_dir_explicit and global_virtual_store_dir_explicit and then call
settings.apply_to(&mut self, &base_dir) so workspace settings can override the
default when present.
---
Outside diff comments:
In `@pacquet/crates/config/src/workspace_yaml.rs`:
- Around line 275-286: The error variants ReadFile and ParseYaml on
LoadWorkspaceYamlError hardcode "pnpm-workspace.yaml", causing misleading
messages when load_global parses other files (e.g., config.yaml); update their
display strings to be file-agnostic (e.g., "Failed to read workspace YAML at {}:
{source}" and "Failed to parse workspace YAML at {}: {source}") so they use the
provided path.display() generically, and apply the same change to any other
occurrences of those variant messages referenced from load_global or similar
loaders.
🪄 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: fe47d3c1-ba3a-4c0f-872c-0696309690df
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
pacquet/crates/config/Cargo.tomlpacquet/crates/config/src/defaults.rspacquet/crates/config/src/defaults/tests.rspacquet/crates/config/src/env_overlay.rspacquet/crates/config/src/lib.rspacquet/crates/config/src/workspace_yaml.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: Agent
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (1)
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: When porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(), orstreamParser.write(), mirror the call site, payload, and ordering so the reporter parses pacquet's NDJSON the same way it parses pnpm's.
Declare a newtype wrapper for branded string types. Do not collapse the brand into a plainStringor&str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only viaTryFrom<String>and/orFromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallibleFrom<String>(andFrom<&str>when convenient).
If upstream occasionally constructs without validation, exposefrom_str_uncheckedas an escape hatch alongside the validating constructor.
Match upstream serde behavior for branded types that cross JSON, YAML, or INI boundaries. Use#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization.
Use#[derive(derive_more::From)]and#[derive(derive_more::Into)]for mechanical conversion impls. Fall back to manualimplonly when conversion needs custom logic.
String-literal unions should becomeenums, not newtype wrappers. Model closed sets of valid string values as enums.
Template literal types should be treated as branded strings with validation discipline from rules 2-5.
Choose owned vs. borrowed parameters to minimize copies. Widen to the most encompassing type (&Pathover&PathBuf,&strover&String) when it doesn't force extra copies.
PreferArc::clone(&x)/Rc::clone(&x)overx.clone()for reference-counted types, so the cost is visible at the call site.
Follow Rust API Guidelines for naming conventions.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;. Two forms stay allowed: external-crate preludes likeuse rayon::prelude::*;and root-of-module re-...
Files:
pacquet/crates/config/src/defaults.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/config/src/defaults/tests.rspacquet/crates/config/src/env_overlay.rspacquet/crates/config/src/lib.rs
Contributor
There was a problem hiding this comment.
Pull request overview
This PR brings pacquet-config closer to pnpm v11’s config precedence by adding two previously-missing sources: the global <configDir>/config.yaml and PNPM_CONFIG_* / pnpm_config_* environment-variable overrides, layered in the same order as upstream (global yaml between .npmrc and pnpm-workspace.yaml, then env vars after workspace yaml).
Changes:
- Add global
<configDir>/config.yamlloading with workspace-only keys stripped for parity with pnpm’sisConfigFileKeyfiltering. - Add an env-var overlay that reads
PNPM_CONFIG_*/pnpm_config_*across the workspace-settings schema and applies it afterpnpm-workspace.yaml. - Harden tests to avoid ambient developer env leakage and add new tests covering the new precedence rules.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pacquet/crates/config/src/workspace_yaml.rs | Adds GLOBAL_CONFIG_YAML_FILENAME, global config loader, and workspace-only field stripping. |
| pacquet/crates/config/src/lib.rs | Wires global config.yaml + env overlay into Config::current; updates/extends tests and env isolation helper. |
| pacquet/crates/config/src/env_overlay.rs | New implementation for parsing PNPM_CONFIG_* / pnpm_config_* into WorkspaceSettings. |
| pacquet/crates/config/src/defaults.rs | Adds default_config_dir matching pnpm’s config-dir resolution chain. |
| pacquet/crates/config/src/defaults/tests.rs | Adds unit tests for default_config_dir behavior. |
| pacquet/crates/config/Cargo.toml | Adds serde_json dependency used by env overlay parsing. |
| Cargo.lock | Updates lockfile for the new dependency. |
💡 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 #11752 +/- ##
==========================================
+ Coverage 89.84% 90.09% +0.24%
==========================================
Files 145 146 +1
Lines 16426 16795 +369
==========================================
+ Hits 14758 15131 +373
+ Misses 1668 1664 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Don't clobber a `virtualStoreDir` set in global `config.yaml` when a `pnpm-workspace.yaml` is present but doesn't redeclare the field. Guard the workspace-root re-anchor on `virtual_store_dir_explicit`. Adds a regression test (CodeRabbit review on #11752). - Drop `parse_tri_array`'s `"null"` shortcut. Pnpm's `Array` schema feeds the env value through `JSON.parse` and then requires `Array.isArray`, so `PNPM_CONFIG_HOIST_PATTERN=null` is silently rejected upstream — accepting it here would diverge from pnpm. Test updated to assert rejection (Copilot review on #11752, but the suggestion direction had to be inverted for parity). - Rename single-letter parameter / generic names (`s`, `T`, `p`) to satisfy perfectionist dylint's `single-letter-*` rules; add trailing comma to a multi-line macro invocation. --- Written by an agent (Claude Code, claude-opus-4-7).
Contributor
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.62735713048,
"stddev": 0.7892127870579948,
"median": 2.4157769564800002,
"user": 2.6842399000000006,
"system": 3.5855290799999993,
"min": 2.2693346824800003,
"max": 4.86658646248,
"times": [
2.4413702764800003,
4.86658646248,
2.2936194374800003,
2.43345121748,
2.37851736548,
2.4319174394800003,
2.2693346824800003,
2.40946657348,
2.32722051048,
2.4220873394800004
]
},
{
"command": "pacquet@main",
"mean": 2.29908109458,
"stddev": 0.05198870125964241,
"median": 2.27931158248,
"user": 2.7084605,
"system": 3.5787768799999986,
"min": 2.2367761944800004,
"max": 2.41877369048,
"times": [
2.2761031864800003,
2.30785164748,
2.2367761944800004,
2.2819742894800004,
2.41877369048,
2.27462549148,
2.2766488754800003,
2.28983302448,
2.3564264394800003,
2.2717981064800004
]
},
{
"command": "pnpm",
"mean": 4.561158989080001,
"stddev": 0.07637164235030787,
"median": 4.544565072979999,
"user": 7.7383262,
"system": 3.9830657799999996,
"min": 4.48327458348,
"max": 4.71918455048,
"times": [
4.55556966848,
4.650131689479999,
4.48327458348,
4.53356047748,
4.49207366848,
4.55847546648,
4.48949885848,
4.52747233448,
4.71918455048,
4.6023485934799995
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.7100549872799999,
"stddev": 0.026174817434079813,
"median": 0.70695747638,
"user": 0.39740732,
"system": 1.5850985999999998,
"min": 0.69024181038,
"max": 0.78083285738,
"times": [
0.78083285738,
0.69536203338,
0.71201721038,
0.69343480938,
0.70558094038,
0.71112102438,
0.70916335638,
0.69024181038,
0.70833401238,
0.69446181838
]
},
{
"command": "pacquet@main",
"mean": 0.7337899571800001,
"stddev": 0.041760137768534975,
"median": 0.71436929488,
"user": 0.39667162,
"system": 1.5986711000000002,
"min": 0.69388047538,
"max": 0.81187050938,
"times": [
0.78404405538,
0.70645541638,
0.71357463538,
0.81187050938,
0.69568257238,
0.70139626938,
0.69388047538,
0.77040510638,
0.71516395438,
0.74542657738
]
},
{
"command": "pnpm",
"mean": 2.43787262418,
"stddev": 0.1478389730163389,
"median": 2.3647389378800003,
"user": 2.93167442,
"system": 2.181939,
"min": 2.3155867043800002,
"max": 2.7812380033800004,
"times": [
2.7812380033800004,
2.57773124238,
2.3607116833800004,
2.52736853838,
2.36117712738,
2.32960171038,
2.3683007483800003,
2.3155867043800002,
2.3989485423800003,
2.3580619413800004
]
}
]
} |
This was referenced May 20, 2026
This was referenced May 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Brings pacquet's config loader closer to pnpm v11 parity by filling two real source-reading gaps surfaced in #11738:
<configDir>/config.yaml— e.g.~/.config/pnpm/config.yaml(Linux),~/Library/Preferences/pnpm/config.yaml(macOS),%LOCALAPPDATA%/pnpm/config/config.yaml(Windows). Loaded between.npmrcandpnpm-workspace.yaml, with workspace-only keys (nodeLinker,hoist,lockfile, …) filtered out to mirror upstream'sisConfigFileKeyallowlist atindex.ts:299-309.PNPM_CONFIG_*/pnpm_config_*env vars for the whole schema (pacquet previously honored onlyNPM_CONFIG_WORKSPACE_DIR). Read for everyWorkspaceSettingsfield and applied afterpnpm-workspace.yamlso env vars win — matching upstream'sparseEnvVarsloop atindex.ts:471-488.Closes the scenario in #11738: a user with
enableGlobalVirtualStore: truein~/.config/pnpm/config.yamlwho runs an install in a project whosepnpm-workspace.yamldoesn't repeat the setting now lands in the global virtual store, same as pnpm.What I deliberately did NOT change
The issue (which I wrote in a prior session) proposed reading
.npmrc,~/.pnpmrc,~/.config/pnpm/rc, andnpm_config_*env vars for the full setting surface. Three of those four are anti-parity:.npmrcisNpmrcReadableKey~/.pnpmrcand~/.config/pnpm/rcnpm_config_*env varsnpm_config_userconfigas an auth-file fallbackPNPM_CONFIG_*is the documented path<configDir>/config.yamlindex.ts:228 / 297-316PNPM_CONFIG_*env varsenv.tsCode map
pacquet/crates/config/src/defaults.rs—default_config_dir<Sys>mirroring upstream'sgetConfigDir.pacquet/crates/config/src/workspace_yaml.rs—WorkspaceSettings::load_global+clear_workspace_only_fields, and theGLOBAL_CONFIG_YAML_FILENAMEconstant.pacquet/crates/config/src/env_overlay.rs(new) —WorkspaceSettings::from_pnpm_config_env::<Sys>()with per-type JSON / string / enum / tri-array parsers.pacquet/crates/config/src/lib.rs— wired both intoConfig::current,|=the GVS-explicit flags across all three sources (global yaml, workspace yaml, env vars), hardened the existing test fakes with asafe_host_varhelper so a developer's real shell env can't leak into hermetic tests.Test plan
cargo nextest run -p pacquet-config— 196/196 pass, including new tests for the issue's exact scenario, workspace-yaml-overrides-global, workspace-only-keys-stripped, env-var-overrides-workspace, lowercase env var spelling, andPNPM_CONFIG_HOIST=falsepost-processing parity.cargo nextest run(full pacquet workspace) — 1486/1486 pass.just check/just lint/just fmt— all clean.typosfailures are pre-existing onmain(unrelated CHANGELOG strings + one comment inverifyLockfileResolutions.ts).Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
New Features
Behavior
Tests