feat(package-manager): version unions in allowBuilds keys (#397 item 5 completion)#428
Conversation
…5 completion) Completes pacquet#397 item 5. Slice A+B's PR #425 moved `AllowBuildPolicy` off `package.json` onto `pnpm-workspace.yaml`; this commit ports the second half of upstream's matcher — the `expandPackageVersionSpecs` step that lets users write keys like `foo@1.0.0 || 2.0.0` in their `allowBuilds` map. ## What this adds New `pacquet_package_manager::version_policy` module porting [`config/version-policy/src/index.ts`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/config/version-policy/src/index.ts) at commit `b4f8f47ac2`. `expand_package_version_specs` parses each `allowBuilds` key into one or more `name` / `name@version` literal strings: - `foo` → `{"foo"}` - `foo@1.0.0` → `{"foo@1.0.0"}` - `foo@1.0.0 || 2.0.0` → `{"foo@1.0.0", "foo@2.0.0"}` - `@scope/foo@1.0.0` → `{"@scope/foo@1.0.0"}` Two error codes mirror upstream: - `ERR_PNPM_INVALID_VERSION_UNION` when a `||` member isn't valid semver - `ERR_PNPM_NAME_PATTERN_IN_VERSION_UNION` when a `*` wildcard in the name is combined with a version part Whitespace around `||` and within each version is trimmed before parsing, matching Node-semver's `valid()`. ## What this does NOT add Wildcards in the name (`is-*`, `@scope/*`) are accepted by the parser and land in the expanded set as literal strings, but `HashSet::contains` lookups mean they never match real package names. Mirrors upstream's `'should not allow patterns in allowBuilds'` test at [`building/policy/test/index.ts:28-34`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/building/policy/test/index.ts#L28-L34) — the original #397 audit incorrectly claimed `@scope/*` should work in `allowBuilds`; it doesn't, neither upstream nor here. `createPackageVersionPolicy` (which DOES support wildcards via `Matcher`) is a separate upstream function used by `minimumReleaseAgeExclude` / `dlx` — pacquet doesn't have those features yet. ## AllowBuildPolicy refactor `AllowBuildPolicy`'s internal storage changes from `HashMap<String, bool>` to two `HashSet<String>` (`expanded_allowed` and `expanded_disallowed`), populated through `expand_package_version_specs`. The `check` function checks `disallowed` before `allowed`, both against bare `name` and `name@version`, mirroring upstream's order at [`building/policy/src/index.ts:35-44`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/building/policy/src/index.ts#L35-L44). This fixes a pre-existing pacquet divergence: the old matcher checked exact-version first, then bare name. With upstream's order, a bare-name disallow now correctly wins over an exact-version allow. New `disallow_bare_name_wins_over_allow_exact_version` test pins the behavior; the old `exact_version_takes_precedence` test was removed (it asserted the divergent behavior). `AllowBuildPolicy::new` now takes the expanded sets directly (pure constructor — no IO). `AllowBuildPolicy::from_config` returns `Result<Self, VersionPolicyError>` so spec-parse failures surface at install time instead of being silently dropped. New `InstallFrozenLockfileError::VersionPolicy` variant propagates the error. ## Tests - 12 new tests in `version_policy::tests` covering: bare name, exact version, version union, scoped names, whitespace trimming in unions, name-with-wildcard-alone (literal), invalid version union (error), mixed valid/invalid (error), wildcard-with-version (error), empty input, duplicate collapse. - Ports of upstream's `building/policy/test/index.ts` cases: `allow_via_version_union` (version-union allows), `wildcard_name_in_allow_builds_does_not_match_real_package` (wildcards inert), `disallow_bare_name_wins_over_allow_exact_version` (matcher order), `disallow_exact_version_with_allow_bare_name` (converse). - `from_config` error-propagation tests for both `VersionPolicyError` variants. 598 tests pass; `just ready`, `just dylint`, `cargo doc -D warnings --document-private-items`, `taplo format --check` all clean. --- 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 (6)
📜 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). (5)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (2)📚 Learning: 2026-05-07T23:19:08.272ZApplied to files:
📚 Learning: 2026-05-01T10:01:33.766ZApplied to files:
🔇 Additional comments (18)
📝 WalkthroughWalkthroughThis PR introduces structured package-version policy expansion by extracting spec parsing into a dedicated module, refactoring ChangesVersion Policy Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #428 +/- ##
==========================================
+ Coverage 86.71% 86.84% +0.12%
==========================================
Files 92 93 +1
Lines 6481 6535 +54
==========================================
+ Hits 5620 5675 +55
+ Misses 861 860 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Ports pnpm’s expandPackageVersionSpecs into pacquet-package-manager so pnpm-workspace.yaml allowBuilds keys can include exact-version unions (e.g. foo@1.0.0 || 2.0.0), and wires parse failures into the frozen-lockfile install path as diagnostics.
Changes:
- Add
version_policymodule withexpand_package_version_specsand upstream-matching diagnostics for invalid unions and wildcard+version combinations. - Refactor
AllowBuildPolicyto store expanded allow/deny sets and to apply upstream precedence (disallow before allow; bare name rules participate). - Update install pipeline + tests to propagate and assert
VersionPolicyErrorbehavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| crates/package-manager/src/version_policy.rs | New parser/expander for <name>@<v1 || v2 …> specs with upstream error codes. |
| crates/package-manager/src/version_policy/tests.rs | Unit tests covering unions, whitespace trimming, wildcard behavior, and error cases. |
| crates/package-manager/src/build_modules.rs | AllowBuildPolicy now uses expanded allow/deny HashSets; from_config becomes fallible; matcher order updated. |
| crates/package-manager/src/build_modules/tests.rs | Test helper updated to mirror runtime expansion; adds union + precedence + error-propagation coverage. |
| crates/package-manager/src/install_frozen_lockfile.rs | Propagates VersionPolicyError via new InstallFrozenLockfileError::VersionPolicy. |
| crates/package-manager/src/lib.rs | Registers and re-exports the new version_policy module. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Micro-Benchmark ResultsLinux |
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.5382036111,
"stddev": 0.1327402480392769,
"median": 2.5224149431000003,
"user": 2.6171872,
"system": 3.4948041599999997,
"min": 2.3917985871,
"max": 2.8525644191,
"times": [
2.6207248550999998,
2.5414053641,
2.3917985871,
2.5021402121,
2.5369920251,
2.5827147780999997,
2.8525644191,
2.4039451141,
2.5078378611,
2.4419128950999998
]
},
{
"command": "pacquet@main",
"mean": 2.4445736677,
"stddev": 0.04817800281937254,
"median": 2.4440112001000003,
"user": 2.6350436,
"system": 3.4297176599999992,
"min": 2.3647051551,
"max": 2.5305952171,
"times": [
2.5305952171,
2.4759975911,
2.4863035371,
2.4431452921,
2.4448771081,
2.4158674131,
2.4052318760999998,
2.4707896701,
2.3647051551,
2.4082238170999997
]
},
{
"command": "pnpm",
"mean": 5.967883844199999,
"stddev": 0.08781308297820856,
"median": 5.971132857100001,
"user": 8.7381174,
"system": 4.308126860000001,
"min": 5.8346596931,
"max": 6.1379277911,
"times": [
5.9205566770999996,
6.0246543941,
5.8346596931,
5.9878447811,
5.9752409961,
5.9670247181,
6.0381853591,
5.9250980721,
5.8676459601,
6.1379277911
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6902099420000001,
"stddev": 0.04409192528382325,
"median": 0.6745310328,
"user": 0.36679118,
"system": 1.4886750599999998,
"min": 0.6599897073000001,
"max": 0.8085515773,
"times": [
0.8085515773,
0.6730349793,
0.6760270863000001,
0.6653408353,
0.6697262603,
0.7133035073,
0.6599897073000001,
0.6849693083,
0.6808567823,
0.6702993763
]
},
{
"command": "pacquet@main",
"mean": 0.784329485,
"stddev": 0.04459400852528666,
"median": 0.7689241648,
"user": 0.35481798,
"system": 1.5364566599999996,
"min": 0.7421109373,
"max": 0.8820290753000001,
"times": [
0.8176427463,
0.7828765183,
0.8820290753000001,
0.7722556873,
0.7451000443,
0.7465010923000001,
0.8244315043,
0.7655926423,
0.7647546023,
0.7421109373
]
},
{
"command": "pnpm",
"mean": 2.452525818,
"stddev": 0.06764214903005424,
"median": 2.4331575773000003,
"user": 2.90059588,
"system": 2.2007328599999996,
"min": 2.3727731053,
"max": 2.5862419393,
"times": [
2.5862419393,
2.4652698353,
2.4054311083,
2.5478660873,
2.4203746463,
2.4661060053,
2.4169909523,
2.3982639923,
2.4459405083,
2.3727731053
]
}
]
} |
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.
Summary
Completes #397 item 5. Slice A+B's PR #425 moved
AllowBuildPolicyoffpackage.jsonontopnpm-workspace.yaml; this PR ports the second half of upstream's matcher —expandPackageVersionSpecs— so users can write keys likefoo@1.0.0 || 2.0.0in theirallowBuildsmap.What this adds
New
pacquet_package_manager::version_policymodule portingconfig/version-policy/src/index.tsat SHAb4f8f47ac2.expand_package_version_specsparses eachallowBuildskey into one or morename/name@versionliteral strings:foo→{"foo"}foo@1.0.0→{"foo@1.0.0"}foo@1.0.0 || 2.0.0→{"foo@1.0.0", "foo@2.0.0"}@scope/foo@1.0.0→{"@scope/foo@1.0.0"}Two error codes mirror upstream:
ERR_PNPM_INVALID_VERSION_UNIONwhen a||member isn't valid semverERR_PNPM_NAME_PATTERN_IN_VERSION_UNIONwhen a*wildcard in the name is combined with a version partWhitespace around
||and within each version is trimmed before parsing, matching Node-semver'svalid().What this does NOT add
Wildcards in the name (
is-*,@scope/*) are accepted by the parser and land in the expanded set as literal strings, butHashSet::containslookups mean they never match real package names. Mirrors upstream's'should not allow patterns in allowBuilds'test — the original #397 audit incorrectly claimed@scope/*should work inallowBuilds; it doesn't, neither upstream nor here.createPackageVersionPolicy(which DOES support wildcards viaMatcher) is a separate upstream function used byminimumReleaseAgeExclude/dlx— pacquet doesn't have those features yet.AllowBuildPolicy refactor
AllowBuildPolicy's internal storage changes fromHashMap<String, bool>to twoHashSet<String>(expanded_allowedandexpanded_disallowed), populated throughexpand_package_version_specs. Thecheckfunction checksdisallowedbeforeallowed, both against barenameandname@version, mirroring upstream's order atbuilding/policy/src/index.ts:35-44.This fixes a pre-existing pacquet divergence: the old matcher checked exact-version first, then bare name. With upstream's order, a bare-name disallow now correctly wins over an exact-version allow. New
disallow_bare_name_wins_over_allow_exact_versiontest pins the behavior; the oldexact_version_takes_precedencetest was removed (it asserted the divergent behavior).AllowBuildPolicy::newnow takes the expanded sets directly (pure constructor — no IO).AllowBuildPolicy::from_configreturnsResult<Self, VersionPolicyError>so spec-parse failures surface at install time instead of being silently dropped. NewInstallFrozenLockfileError::VersionPolicyvariant propagates the error.Test plan
version_policy::testscovering: bare name, exact version, version union, scoped names, whitespace trimming in unions, name-with-wildcard-alone (literal), invalid version union (error), mixed valid/invalid (error), wildcard-with-version (error), empty input, duplicate collapse.building/policy/test/index.tscases:allow_via_version_union(version-union allows),wildcard_name_in_allow_builds_does_not_match_real_package(wildcards inert),disallow_bare_name_wins_over_allow_exact_version(matcher order),disallow_exact_version_with_allow_bare_name(converse).from_configerror-propagation tests for bothVersionPolicyErrorvariants.just readyclean — 598 tests pass.just dylint(perfectionist) clean.RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --workspace --document-private-itemsclean.taplo format --checkclean.Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
Bug Fixes
Refactor