feat: patchedDependencies foundation + cache key + AllowBuildPolicy from Config (#397 items 9 A+B, 5)#425
Conversation
… slice A) Add a new `pacquet-patching` crate porting the upstream `@pnpm/patching.types` + `@pnpm/patching.config` workspaces and the patch-file hashing in `@pnpm/lockfile.settings-checker.calcPatchHashes` (pinned at SHA b4f8f47ac2): - `PatchInfo`, `ExtendedPatchInfo`, `PatchGroupRangeItem`, `PatchGroup`, `PatchGroupRecord` mirroring upstream's `patching/types/src/index.ts`. - `parse_key`: minimal port of `dp.parse` covering the `name[@Version]` subset that patchedDependencies keys use. - `group_patched_dependencies` with `ERR_PNPM_PATCH_NON_SEMVER_RANGE`. - `get_patch_info` (exact → unique-range → wildcard) with `ERR_PNPM_PATCH_KEY_CONFLICT`. - `all_patch_keys` and `verify_patches` with `ERR_PNPM_UNUSED_PATCH`, returning a structured `UnusedPatches` payload when the caller passes `allow_unused_patches: true` so it can warn via `pacquet_diagnostics`. - `create_hex_hash_from_file` / `calc_patch_hashes`: SHA-256 hex with upstream's CRLF→LF normalization. The normalization matters: a patch file authored on Windows would otherwise hash differently than the same file on POSIX, and the resulting `patch_hash` would change between platforms. - `load_patched_dependencies_from_manifest`: reads `pnpm.patchedDependencies` from `package.json`, resolves relative patch paths against the manifest dir (mirroring `getOptionsFromPnpmSettings`), hashes each file, and buckets them via `group_patched_dependencies`. Slice A is pure foundation — nothing in the install pipeline consumes the new crate yet. Slice B will thread `Option<PatchInfo>` per snapshot into `BuildModules` (`requires_build || patch.is_some()` build trigger) and into the side-effects-cache key (`build_modules.rs:326`, `patch_file_hash: None` placeholder). Slice C will apply patches to extracted package dirs before postinstall hooks. Tests port the upstream coverage in `patching/config/test/{groupPatchedDependencies,getPatchInfo}.test.ts` plus new cases for the manifest loader, CRLF normalization, and the error paths. --- 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 (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 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-01T10:01:33.766ZApplied to files:
📚 Learning: 2026-05-07T23:19:08.272ZApplied to files:
🔇 Additional comments (6)
📝 WalkthroughWalkthroughAdds a new ChangesPatching configuration and logic
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #425 +/- ##
==========================================
+ Coverage 86.33% 86.68% +0.35%
==========================================
Files 85 91 +6
Lines 6123 6344 +221
==========================================
+ Hits 5286 5499 +213
- Misses 837 845 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Micro-Benchmark ResultsLinux |
… package.json pnpm v11 stopped reading install settings from package.json's `pnpm` field. `patchedDependencies` (along with `allowBuilds`, `overrides`, etc.) lives in `pnpm-workspace.yaml`. The audit anchored on upstream's misleadingly-named `config/reader/src/getOptionsFromRootManifest.ts`, whose call site at config/reader/src/index.ts:814 actually passes the *workspace* manifest. Only `releasing/commands/src/pack-app/packApp.ts` still reads `manifest.pnpm`, and only for `pnpm.app` — unrelated to install settings. Changes: - Drop `load_patched_dependencies_from_manifest` and its tests; the package.json-reading path is gone entirely. - Add `resolve_and_group(workspace_dir, raw_map)` in pacquet-patching: takes a pre-parsed `BTreeMap<String, String>`, resolves relative paths against the workspace dir, hashes each file via `create_hex_hash_from_file`, and buckets the entries with `group_patched_dependencies`. Mirrors the workspace-dir-resolution + hashing half of upstream's `getOptionsFromPnpmSettings` composed with the `calcPatchHashes` step in `installing/deps-installer`. - Add `patched_dependencies: Option<BTreeMap<String, String>>` on `WorkspaceSettings` so pnpm-workspace.yaml's `patchedDependencies` key deserializes. Not yet pushed to `Config` — that wiring lands in slice B alongside the build-trigger work. - Drop `serde_json` from pacquet-patching's deps (no longer reads JSON). --- Written by an agent (Claude Code, claude-opus-4-7).
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.47619762238,
"stddev": 0.09866053269702903,
"median": 2.4602018385799997,
"user": 2.5831018400000003,
"system": 3.4129157,
"min": 2.34159969858,
"max": 2.64070460158,
"times": [
2.43003239658,
2.58478664958,
2.64070460158,
2.41895455358,
2.56902622658,
2.49037128058,
2.38025091258,
2.50852294658,
2.34159969858,
2.3977269575799998
]
},
{
"command": "pacquet@main",
"mean": 2.41982490458,
"stddev": 0.06317413470937525,
"median": 2.39963501808,
"user": 2.5835342399999996,
"system": 3.3971671,
"min": 2.34304871958,
"max": 2.55032131758,
"times": [
2.37952780858,
2.39442360658,
2.34304871958,
2.48967813058,
2.40484642958,
2.36355588358,
2.45761897558,
2.55032131758,
2.42670770958,
2.38852046458
]
},
{
"command": "pnpm",
"mean": 5.83009096738,
"stddev": 0.06309412653382651,
"median": 5.81116651158,
"user": 8.475599840000001,
"system": 4.3129114,
"min": 5.7323670375799995,
"max": 5.95945346858,
"times": [
5.8129647135799996,
5.80074629358,
5.95945346858,
5.78176912858,
5.80822080958,
5.7323670375799995,
5.88901942458,
5.87049279558,
5.83650769258,
5.80936830958
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.67605990248,
"stddev": 0.032039555192206484,
"median": 0.6743068838800002,
"user": 0.34197854,
"system": 1.4904739999999999,
"min": 0.6442306733800001,
"max": 0.74997295638,
"times": [
0.74997295638,
0.6461175413800001,
0.6682021023800001,
0.68342634538,
0.6891986883800001,
0.6804116653800001,
0.6442306733800001,
0.69508584638,
0.65624516838,
0.64770803738
]
},
{
"command": "pacquet@main",
"mean": 0.6887290717800001,
"stddev": 0.04078388614968822,
"median": 0.67870961738,
"user": 0.34311324,
"system": 1.5048989,
"min": 0.6493318383800001,
"max": 0.78027504538,
"times": [
0.7252673593800001,
0.69190099538,
0.69471311438,
0.6623669293800001,
0.6655182393800001,
0.78027504538,
0.6569178523800001,
0.7057494213800001,
0.65524992238,
0.6493318383800001
]
},
{
"command": "pnpm",
"mean": 2.5001719393799995,
"stddev": 0.13125735784451253,
"median": 2.4362225753799995,
"user": 2.8718945400000004,
"system": 2.1887244,
"min": 2.37056113338,
"max": 2.7324398733799997,
"times": [
2.45102335038,
2.41629506438,
2.7324398733799997,
2.56057533638,
2.41122994338,
2.4214218003799997,
2.3995982403799996,
2.52283701538,
2.37056113338,
2.71573763638
]
}
]
} |
CI's Dylint job (perfectionist::macro-trailing-comma) requires a trailing comma on the last argument of every multi-line macro invocation. `cargo fmt` does not add it, so freshly-formatted code that passes `just ready` locally can still fail CI. Add the missing commas in the multi-line assert_eq! and vec! calls introduced in this slice. `just dylint` now passes locally. Run https://github.com/pnpm/pacquet/actions/runs/25750162090. --- Written by an agent (Claude Code, claude-opus-4-7).
CI's Doc job runs `RUSTDOCFLAGS="-D warnings" cargo doc --no-deps
--workspace` and fails on `[\`pacquet_diagnostics\`]` in verify.rs:59
and verify/tests.rs:40 — pacquet-diagnostics isn't a dep of
pacquet-patching, so rustdoc can't resolve the link. The comments
are just informational ("the caller can warn via X"); demote to
plain code text.
Run https://github.com/pnpm/pacquet/actions/runs/25751011868.
---
Written by an agent (Claude Code, claude-opus-4-7).
There was a problem hiding this comment.
Pull request overview
Introduces a new foundation crate (pacquet-patching) to support pnpm’s patchedDependencies configuration (types, parsing/grouping, matching, verification, and patch-file hashing), and extends workspace-yaml config parsing to capture patchedDependencies from pnpm-workspace.yaml. This is infrastructure-only; nothing in the install pipeline consumes it yet.
Changes:
- Add
pacquet-patchingcrate implementing patch key parsing, grouping, match selection, verification of unused patches, and CRLF-normalized SHA-256 hashing (with unit tests). - Add
WorkspaceSettings.patched_dependenciesto deserializepatchedDependenciesfrompnpm-workspace.yaml(with tests). - Register the new crate in workspace dependencies and lockfile.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/patching/Cargo.toml | New crate manifest for pacquet-patching with required deps. |
| crates/patching/src/lib.rs | Crate-level docs and public re-exports for patching foundation APIs. |
| crates/patching/src/types.rs | Data model mirroring upstream patching types (PatchInfo, PatchGroup*). |
| crates/patching/src/key.rs | Parser for patchedDependencies keys (name[@version] subset). |
| crates/patching/src/group.rs | Bucketing/grouping logic for configured patches; non-semver range error. |
| crates/patching/src/group/tests.rs | Unit tests ported from upstream grouping behavior. |
| crates/patching/src/get_patch_info.rs | Patch selection logic (exact → unique-range → wildcard) with conflict error. |
| crates/patching/src/get_patch_info/tests.rs | Unit tests ported from upstream matcher behavior. |
| crates/patching/src/hash.rs | Patch hashing with CRLF→LF normalization; calc hashes helper + tests. |
| crates/patching/src/resolve.rs | Helper to resolve patch paths against workspace dir, hash, then group. |
| crates/patching/src/resolve/tests.rs | Unit tests for resolution + hashing + grouping integration. |
| crates/patching/src/verify.rs | Verification utilities for unused configured patches + error/warn payload. |
| crates/patching/src/verify/tests.rs | Unit tests for unused-patch verification behavior. |
| crates/config/src/workspace_yaml.rs | Add patched_dependencies field to WorkspaceSettings to parse YAML. |
| crates/config/src/workspace_yaml/tests.rs | Add tests covering presence/absence and shape of patchedDependencies. |
| Cargo.toml | Add pacquet-patching to [workspace.dependencies]. |
| Cargo.lock | Record new workspace package pacquet-patching. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/patching/src/resolve/tests.rs (1)
61-74: ⚡ Quick winAdd assertion context to non-
assert_eq!checks in tests.Please include context in these
assert!calls so failures show the actual value/error immediately.Example targeted changes
- assert!(matches!(err, ResolvePatchedDependenciesError::Hash(_))); + assert!( + matches!(err, ResolvePatchedDependenciesError::Hash(_)), + "expected Hash error, got: {err:?}" + ); @@ - assert!(matches!(err, ResolvePatchedDependenciesError::Range(_))); + assert!( + matches!(err, ResolvePatchedDependenciesError::Range(_)), + "expected Range error, got: {err:?}" + ); @@ - assert!(foo.exact.contains_key("1.0.0")); + assert!( + foo.exact.contains_key("1.0.0"), + "missing exact key 1.0.0 in foo group: {foo:?}" + ); @@ - assert!(bar.all.is_some()); + assert!(bar.all.is_some(), "missing wildcard patch in bar group: {bar:?}");Based on learnings: “if you use assertions like
assert!,assert_ne!, etc., ensure the test logs the relevant actual/expected values (or context).”Also applies to: 96-100
🤖 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/patching/src/resolve/tests.rs` around lines 61 - 74, Update the tests to include assertion context on the non-assert_eq! checks: for the match assertions in the tests (e.g., the one in invalid_version_range_propagates that asserts matches!(err, ResolvePatchedDependenciesError::Range(_)) and the earlier one checking ResolvePatchedDependenciesError::Hash(_)), change them to pass a diagnostic message that prints the actual err (for example using a debug-format string like "err = {:?}", err) so failures surface the actual value; apply the same pattern to the other similar assertions around lines 96-100 as well.crates/patching/src/verify/tests.rs (1)
27-29: ⚡ Quick winDon’t sort here—assert the stable order contract directly.
Sorting hides ordering regressions in
all_patch_keys, which is explicitly documented as stable.Suggested test adjustment
- let mut keys: Vec<&str> = all_patch_keys(&groups).collect(); - keys.sort(); + let keys: Vec<&str> = all_patch_keys(&groups).collect(); - assert_eq!(keys, vec!["bar@3.0.0", "baz", "foo", "foo@1.0.0", "foo@^2.0.0"]); + assert_eq!( + keys, + vec!["bar@3.0.0", "baz", "foo@1.0.0", "foo@^2.0.0", "foo"] + );🤖 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/patching/src/verify/tests.rs` around lines 27 - 29, The test is hiding ordering bugs by sorting the results; remove the call to keys.sort() and assert the exact stable order returned by all_patch_keys(&groups) directly. Update the assertion against keys (the Vec<&str> produced by all_patch_keys) to the expected stable order (preserve the intended ordering that the crate documents) so the test fails on regressions; refer to the all_patch_keys function and the keys variable when making this change.crates/patching/src/hash.rs (1)
124-132: ⚡ Quick winAdd failure context for
assert!(matches!(...))tests.For these assertions, include the actual error in the assertion message so failures are diagnosable without reruns.
Small test diagnostics improvement
- assert!(matches!(err, super::CalcPatchHashError::ReadFile { .. })); + assert!( + matches!(err, super::CalcPatchHashError::ReadFile { .. }), + "expected ReadFile, got: {err:?}" + ); @@ - assert!(matches!(err, super::CalcPatchHashError::NotUtf8 { .. })); + assert!( + matches!(err, super::CalcPatchHashError::NotUtf8 { .. }), + "expected NotUtf8, got: {err:?}" + );Based on learnings: “if you use assertions like
assert!,assert_ne!, etc., ensure the test logs the relevant actual/expected values (or context).”Also applies to: 135-141
🤖 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/patching/src/hash.rs` around lines 124 - 132, The test missing_file_errors (and the similar test around lines 135-141) currently uses assert!(matches!(err, super::CalcPatchHashError::ReadFile { .. })), which hides the actual error on failure; update these assertions to include the actual error in the assertion message so failures are diagnosable (e.g., replace the bare assert! with assert!(matches!(err, super::CalcPatchHashError::ReadFile { .. }), "unexpected error: {:?}", err)). Locate usages of create_hex_hash_from_file and the CalcPatchHashError::ReadFile pattern in those tests and add the formatted err to the assertion message.
🤖 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/patching/src/hash.rs`:
- Around line 17-22: The NotUtf8 error variant in the Patch file parsing (enum
variant NotUtf8 in hash.rs) must be removed and the code changed to use lossy
UTF-8 decoding instead of String::from_utf8() so invalid bytes are replaced with
U+FFFD (matching Node's fs.readFile('utf8')) — update any code paths that
construct or return NotUtf8 to instead decode with
String::from_utf8_lossy(&bytes).into_owned() (or equivalent) and remove the
NotUtf8 variant and its FromUtf8Error source field; also delete the
non_utf8_errors() test that expects an error for invalid UTF-8. Ensure
references to the removed variant (e.g., error construction or pattern matches)
are updated to the new flow.
---
Nitpick comments:
In `@crates/patching/src/hash.rs`:
- Around line 124-132: The test missing_file_errors (and the similar test around
lines 135-141) currently uses assert!(matches!(err,
super::CalcPatchHashError::ReadFile { .. })), which hides the actual error on
failure; update these assertions to include the actual error in the assertion
message so failures are diagnosable (e.g., replace the bare assert! with
assert!(matches!(err, super::CalcPatchHashError::ReadFile { .. }), "unexpected
error: {:?}", err)). Locate usages of create_hex_hash_from_file and the
CalcPatchHashError::ReadFile pattern in those tests and add the formatted err to
the assertion message.
In `@crates/patching/src/resolve/tests.rs`:
- Around line 61-74: Update the tests to include assertion context on the
non-assert_eq! checks: for the match assertions in the tests (e.g., the one in
invalid_version_range_propagates that asserts matches!(err,
ResolvePatchedDependenciesError::Range(_)) and the earlier one checking
ResolvePatchedDependenciesError::Hash(_)), change them to pass a diagnostic
message that prints the actual err (for example using a debug-format string like
"err = {:?}", err) so failures surface the actual value; apply the same pattern
to the other similar assertions around lines 96-100 as well.
In `@crates/patching/src/verify/tests.rs`:
- Around line 27-29: The test is hiding ordering bugs by sorting the results;
remove the call to keys.sort() and assert the exact stable order returned by
all_patch_keys(&groups) directly. Update the assertion against keys (the
Vec<&str> produced by all_patch_keys) to the expected stable order (preserve the
intended ordering that the crate documents) so the test fails on regressions;
refer to the all_patch_keys function and the keys variable when making this
change.
🪄 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: d2f348ce-f603-4ca6-a562-ba9e7cb2c44e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
Cargo.tomlcrates/config/src/workspace_yaml.rscrates/config/src/workspace_yaml/tests.rscrates/patching/Cargo.tomlcrates/patching/src/get_patch_info.rscrates/patching/src/get_patch_info/tests.rscrates/patching/src/group.rscrates/patching/src/group/tests.rscrates/patching/src/hash.rscrates/patching/src/key.rscrates/patching/src/lib.rscrates/patching/src/resolve.rscrates/patching/src/resolve/tests.rscrates/patching/src/types.rscrates/patching/src/verify.rscrates/patching/src/verify/tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Agent
- 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: Preserve existing method chains andpipe-traitchains; do not break them into intermediateletbindings unless there is a concrete justification such as a compilation failure, borrow checker rejection, meaningful performance improvement, or other technical necessity. Refactoring for style alone is not sufficient justification.
Choose owned vs. borrowed parameters to minimize copies; prefer borrowed types (&Pathover&PathBuf,&strover&String) when it does not force extra copies.
PreferArc::clone(&x)andRc::clone(&x)overx.clone()for reference-counted types to make the cost visible at the call site.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;for any glob whose target is a module you control. External-crate preludes (e.g.,use rayon::prelude::*;) and root-of-module re-exports (e.g.,pub use submodule::*;inlib.rs) are exceptions.
Follow Rust API Guidelines for naming, as documented in https://rust-lang.github.io/api-guidelines/naming.html.
Declare a newtype wrapper for any branded string type being ported from TypeScript pnpm. Do not collapse the brand into a plainStringor&str; give the type its own struct so misuse is a type error.
When porting branded string types where upstream TypeScript always validates before construction, validate in the Rust port too. Construct the wrapper only viaTryFrom<String>and/orFromStr; do not provide an infallible public constructor that takes an arbitrary string.
For branded string types where upstream TypeScript never validates (used purely for type-safety to prevent confusion between string slots), expose an infallibleFrom<String>andFrom<&str>constructor in the Rust wrapper.
When upstream TypeScript occasionally constructs a branded type without validation (via bareasassertion), add afrom_str_unchecked(or similarly named) constructor on the Rust side. Keep the validating constructor as well; `from_str_u...
Files:
crates/config/src/workspace_yaml/tests.rscrates/patching/src/group.rscrates/patching/src/verify.rscrates/config/src/workspace_yaml.rscrates/patching/src/resolve.rscrates/patching/src/group/tests.rscrates/patching/src/verify/tests.rscrates/patching/src/get_patch_info.rscrates/patching/src/resolve/tests.rscrates/patching/src/lib.rscrates/patching/src/hash.rscrates/patching/src/types.rscrates/patching/src/get_patch_info/tests.rscrates/patching/src/key.rs
🧠 Learnings (2)
📚 Learning: 2026-05-01T10:01:33.766Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 349
File: crates/reporter/src/tests.rs:121-121
Timestamp: 2026-05-01T10:01:33.766Z
Learning: In Rust test code, follow the repo’s CODE_STYLE_GUIDE test-logging rule: add logging (e.g., `eprintln!`/`eprintln!(...)`) so that useful diagnostic values are printed when a test fails, unless the assertion is `assert_eq!` (where the differing values are already included). Concretely, if you use assertions like `assert!`, `assert_ne!`, etc., ensure the test logs the relevant actual/expected values (or context) before/around the assertion so failures can be diagnosed without rerunning.
Applied to files:
crates/config/src/workspace_yaml/tests.rscrates/patching/src/group/tests.rscrates/patching/src/verify/tests.rscrates/patching/src/resolve/tests.rscrates/patching/src/get_patch_info/tests.rs
📚 Learning: 2026-05-07T23:19:08.272Z
Learnt from: KSXGitHub
Repo: pnpm/pacquet PR: 401
File: tasks/integrated-benchmark/src/work_env.rs:343-344
Timestamp: 2026-05-07T23:19:08.272Z
Learning: When reviewing Rust code in pnpm/pacquet for deprecated API usage, do not automatically treat `serde_saphyr::to_string` as deprecated. In `serde-saphyr` v0.0.25, `serde_saphyr::to_string` has no `#[deprecated]` attribute (the `#[deprecated]` later in `serde-saphyr-0.0.25/src/lib.rs` applies to a different function). Only flag `serde_saphyr::to_string` as deprecated if the resolved dependency version’s source shows `#[deprecated]` on that specific function.
Applied to files:
crates/config/src/workspace_yaml/tests.rscrates/patching/src/group.rscrates/patching/src/verify.rscrates/config/src/workspace_yaml.rscrates/patching/src/resolve.rscrates/patching/src/group/tests.rscrates/patching/src/verify/tests.rscrates/patching/src/get_patch_info.rscrates/patching/src/resolve/tests.rscrates/patching/src/lib.rscrates/patching/src/hash.rscrates/patching/src/types.rscrates/patching/src/get_patch_info/tests.rscrates/patching/src/key.rs
🔇 Additional comments (14)
crates/config/src/workspace_yaml.rs (1)
70-81: Excellent documentation and clean integration.The
patched_dependenciesfield is well-documented with upstream references and correctly typed asOption<BTreeMap<String, String>>. The documentation clearly explains that path resolution and hashing are deferred topacquet_patching::resolve_and_group, keeping this layer as pure deserialized data. The field is intentionally not yet wired intoapply_toper the PR objectives (slice B follow-up).crates/config/src/workspace_yaml/tests.rs (1)
191-218: LGTM! Comprehensive deserialization coverage.The tests validate the critical deserialization behaviors:
patchedDependenciesYAML field maps topatched_dependenciesstruct field via camelCase rename- Keys with optional
@versionsuffixes are preserved as-is- Values are captured as string paths
- Absence of the field yields
Nonecrates/patching/Cargo.toml (1)
1-22: Clean crate manifest with proper workspace integration.All dependencies correctly reference workspace versions, and the set of dependencies matches the implementation needs (derive_more/miette for error types, node-semver for version parsing, sha2 for hashing).
Cargo.toml (1)
33-33: LGTM!The workspace dependency declaration for
pacquet-patchingcorrectly follows the existing pattern.crates/patching/src/types.rs (1)
1-65: Well-designed type definitions with appropriate collection choices.The use of
BTreeMapforPatchGroup.exact(deterministic iteration for error messages and lockfile stability) andVecforPatchGroup.range(preserves user ordering) shows careful consideration of the trade-offs. Documentation thoroughly links to upstream equivalents.crates/patching/src/group.rs (2)
51-98: Faithful port of upstream grouping logic.The four-way routing (exact → range/star → bare name) correctly mirrors upstream's
groupPatchedDependencies. The precedence order and wildcard handling match the documented upstream behavior.
73-77: No changes needed. The error path is correctly validated by the test at lines 101–105 (errors_on_invalid_version_range), which confirms thatRange::parsefromnode-semver(v2.2.0) rejects protocol specifiers like"link:packages/foo".crates/patching/src/group/tests.rs (1)
1-125: Excellent test coverage mirroring upstream test suite.The tests comprehensively validate grouping behavior across all routing paths (exact, range, star, bare name, mixed, and error cases). The test structure and naming faithfully mirror upstream's test suite, which will make cross-referencing and future maintenance easier.
crates/patching/src/key.rs (1)
43-47: Version::parse correctly distinguishes exact versions from ranges; no action needed.The code uses
node_semver::Version::parse(v2.2.0) which accepts exact semver strings and rejects range syntax (^,~,x.xpatterns, etc.). The test cases confirm this:parse_key("lodash@^4.17.0")correctly yieldsnon_semver_version, andparse_key("lodash@4.17.21")correctly yieldsversion. The implementation is correct.crates/patching/src/get_patch_info.rs (1)
46-90: Patch selection precedence implementation looks correct.Exact → unique range → wildcard flow is clear and consistent with the intended semantics.
crates/patching/src/get_patch_info/tests.rs (1)
17-143: Test matrix is solid for matcher precedence and conflict behavior.crates/patching/src/resolve.rs (1)
53-75: Path resolution + hash/group pipeline looks good and cleanly composed.crates/patching/src/verify.rs (1)
14-23: Verification flow and unused-patch branching look correct.Also applies to: 61-78
crates/patching/src/lib.rs (1)
22-36: Public module/re-export surface is well organized and clear.
items 9 slice B, 5) Slice B of pacquet#397 item 9 — wire `patchedDependencies` through the install pipeline as far as the side-effects-cache key, and (per #397 item 5) move `AllowBuildPolicy` off `package.json` onto `pnpm-workspace.yaml`. The build-trigger update and actual patch application are deferred to slice C — they only make sense paired together. ## Config + WorkspaceSettings - `Config.workspace_dir: Option<PathBuf>` — the dir containing the nearest ancestor `pnpm-workspace.yaml`, recorded by `WorkspaceSettings::apply_to` so install-time code can resolve patch file paths against the same dir upstream uses. - `Config.patched_dependencies: Option<BTreeMap<String, String>>` — raw `patchedDependencies` from `pnpm-workspace.yaml`. Hashing + path resolution is deferred to `Config::resolved_patched_dependencies` so the apply_to layer stays infallible. - `Config.allow_builds: HashMap<String, bool>` and `Config.dangerously_allow_all_builds: bool` — replace `AllowBuildPolicy::from_manifest`'s `package.json` reads (pnpm v11 moved these to `pnpm-workspace.yaml`, see #397 item 5). - `WorkspaceSettings` gains the matching three fields; `apply_to` pushes them. - New `Config::resolved_patched_dependencies()` runs `pacquet_patching::resolve_and_group` on demand. ## BuildModules - New `patches: Option<&HashMap<PackageKey, ExtendedPatchInfo>>` field on `BuildModules`, keyed by the peer-stripped `PackageKey` (patches are configured at name+version granularity, not per peer-resolution variant). - The `cache_key` computation at the existing `build_modules.rs:326` placeholder now passes `patch_file_hash: patch.map(|p| p.hash.as_str())` to `calc_dep_state`, matching upstream's `patchFileHash: depNode.patch?.hash` at https://github.com/pnpm/pnpm/blob/b4f8f47ac2/building/during-install/src/index.ts#L201. ## AllowBuildPolicy (#397 item 5) - Drop `AllowBuildPolicy::from_manifest`. The package.json reader was a workaround until Config plumbing landed. - Add `AllowBuildPolicy::from_config(&Config)` — reads `config.allow_builds` and `config.dangerously_allow_all_builds`, which workspace_yaml.rs now populates from `pnpm-workspace.yaml`. - `install_frozen_lockfile.rs:156` switches over. ## Install pipeline - `InstallFrozenLockfile::run` calls `config.resolved_patched_dependencies()` once, then computes a per-snapshot `HashMap<PackageKey, ExtendedPatchInfo>` via `pacquet_patching::get_patch_info` against each snapshot's `(name, version)`. Mirrors upstream's per-resolution lookup at https://github.com/pnpm/pnpm/blob/b4f8f47ac2/pkg-manager/resolve-dependencies/src/resolveDependencies.ts#L1482, adapted for pacquet's lockfile-driven flow (snapshots come from the lockfile after resolution). ## Tests - New `write_path_cache_key_includes_patch_hash` confirms the threading end-to-end: given a `patches` map with a specific hash, the side-effects-cache row's key contains `;patch=<hash>`. - New `apply_pushes_patched_dependencies_and_workspace_dir`, `parses_allow_builds_from_yaml_and_applies`, `parses_dangerously_allow_all_builds_from_yaml_and_applies` for the yaml-side wiring. - `missing_manifest_denies_all` / `from_manifest_parses_pnpm_section` become `empty_config_denies_all` / `from_config_consumes_allow_builds_and_dangerously_allow_all_builds`, reflecting the new source of truth. ## What's deferred to slice C The build trigger update (`requires_build || patch.is_some()`) and the actual patch application both belong in slice C. Wiring them in isolation produces an incoherent state: filtering chunks to include patched packages without also applying the patch would either skip the build entirely or run scripts against an unpatched dir, and the cache key would advertise a patch that isn't on disk. Both land together in slice C with the applier decision (Rust crate vs `git apply`) raised then. --- Written by an agent (Claude Code, claude-opus-4-7).
Both CodeRabbit and Copilot flagged two substantive issues on the slice A SHA (66e24b7): 1. `create_hex_hash_from_file` errored on non-UTF-8 patch files via `String::from_utf8`. Node `fs.readFile(path, 'utf8')` (upstream's `readNormalizedFile`) replaces invalid bytes with U+FFFD and never throws, so the strict variant introduced a behavior drift: a patch file with stray non-UTF-8 bytes works under pnpm but failed under pacquet. Switched to `String::from_utf8_lossy(&bytes).into_owned()` and dropped the `NotUtf8` error variant. 2. `patched_dependencies` was deserialized into a `BTreeMap`, which sorts keys alphabetically and loses the order users wrote in `pnpm-workspace.yaml`. Range entries inside `PatchGroup.range` ended up alphabetical instead of user-listed; that surfaces in `PATCH_KEY_CONFLICT` diagnostics where pnpm lists matched ranges in user order. Switched `WorkspaceSettings.patched_dependencies`, `Config.patched_dependencies`, and `resolve_and_group`'s input to `IndexMap<String, String>`. `PatchGroupRecord`'s outer `BTreeMap` and `PatchGroup.exact` remain alphabetical — they don't feed user-visible diagnostics ordered output. Also applied CodeRabbit's nits: - Added `, "got: {err:?}"` to `assert!(matches!(err, ...))` calls in `hash.rs` and `resolve/tests.rs`. - Removed `keys.sort()` from `verify::tests::all_keys_yields_every_configured_key` and asserted the stable order directly: outer BTreeMap iteration (alphabetical by package name) then exact/range/all within each group. New tests: - `non_utf8_uses_replacement_char_and_does_not_error` — pins the SHA-256 of three U+FFFD chars (cross-checked against `printf '\xef\xbf\xbd\xef\xbf\xbd\xef\xbf\xbd' | shasum -a 256`). - `range_preserves_user_specified_order` — input listed as `~1.2.0`, `4`, `>=5 <6` lands in `PatchGroup.range` in that order. A regression to `BTreeMap` would alphabetize to `4`, `>=5 <6`, `~1.2.0` and fail. 566 tests pass; `just ready`, `just dylint`, `cargo doc -D warnings`, `taplo format --check` all clean. Addresses comments in #425 review at #425 (review) (reviewing 66e24b7). --- Written by an agent (Claude Code, claude-opus-4-7).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/patching/src/hash.rs (1)
22-24: ⚡ Quick winUse
pnpm/blob/mainlinks in porting references (not pinned SHA permalinks).These doc links currently point to a fixed commit SHA. Please switch them to
https://github.com/pnpm/pnpm/blob/main/...so future cross-checks follow current upstream source-of-truth.Suggested doc-link update
-/// [`createHexHashFromFile`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/crypto/hash/src/index.ts#L31-L33) +/// [`createHexHashFromFile`](https://github.com/pnpm/pnpm/blob/main/crypto/hash/src/index.ts#L31-L33) /// composed with -/// [`readNormalizedFile`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/crypto/hash/src/index.ts#L36-L39). +/// [`readNormalizedFile`](https://github.com/pnpm/pnpm/blob/main/crypto/hash/src/index.ts#L36-L39). -/// [`calcPatchHashes`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/lockfile/settings-checker/src/calcPatchHashes.ts). +/// [`calcPatchHashes`](https://github.com/pnpm/pnpm/blob/main/lockfile/settings-checker/src/calcPatchHashes.ts). - /// [`readNormalizedFile`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/crypto/hash/src/index.ts#L36-L39). + /// [`readNormalizedFile`](https://github.com/pnpm/pnpm/blob/main/crypto/hash/src/index.ts#L36-L39). - /// [`readNormalizedFile`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/crypto/hash/src/index.ts#L36-L39). + /// [`readNormalizedFile`](https://github.com/pnpm/pnpm/blob/main/crypto/hash/src/index.ts#L36-L39).As per coding guidelines, "If reading on GitHub, open files from
https://github.com/pnpm/pnpm/blob/main/...rather than from a permalinked SHA."Also applies to: 50-50, 90-90, 137-137
🤖 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/patching/src/hash.rs` around lines 22 - 24, Update the GitHub doc links in the comments that reference pnpm source to use the live main branch instead of a pinned commit SHA: replace URLs like https://github.com/pnpm/pnpm/blob/b4f8f47ac2/... with https://github.com/pnpm/pnpm/blob/main/... for the references to createHexHashFromFile and readNormalizedFile (and the other occurrences noted around lines referenced in the review), ensuring all three comment links are changed so future readers follow the current upstream source-of-truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/patching/src/hash.rs`:
- Around line 22-24: Update the GitHub doc links in the comments that reference
pnpm source to use the live main branch instead of a pinned commit SHA: replace
URLs like https://github.com/pnpm/pnpm/blob/b4f8f47ac2/... with
https://github.com/pnpm/pnpm/blob/main/... for the references to
createHexHashFromFile and readNormalizedFile (and the other occurrences noted
around lines referenced in the review), ensuring all three comment links are
changed so future readers follow the current upstream source-of-truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0172c79b-6467-4f73-a0ef-45eac3e40321
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
crates/config/Cargo.tomlcrates/config/src/lib.rscrates/config/src/workspace_yaml.rscrates/patching/Cargo.tomlcrates/patching/src/hash.rscrates/patching/src/resolve.rscrates/patching/src/resolve/tests.rscrates/patching/src/verify/tests.rs
✅ Files skipped from review due to trivial changes (3)
- crates/config/Cargo.toml
- crates/patching/src/resolve/tests.rs
- crates/patching/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/config/src/lib.rs
- crates/patching/src/resolve.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). (6)
- GitHub Check: Agent
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Run benchmark on ubuntu-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: Preserve existing method chains andpipe-traitchains; do not break them into intermediateletbindings unless there is a concrete justification such as a compilation failure, borrow checker rejection, meaningful performance improvement, or other technical necessity. Refactoring for style alone is not sufficient justification.
Choose owned vs. borrowed parameters to minimize copies; prefer borrowed types (&Pathover&PathBuf,&strover&String) when it does not force extra copies.
PreferArc::clone(&x)andRc::clone(&x)overx.clone()for reference-counted types to make the cost visible at the call site.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;for any glob whose target is a module you control. External-crate preludes (e.g.,use rayon::prelude::*;) and root-of-module re-exports (e.g.,pub use submodule::*;inlib.rs) are exceptions.
Follow Rust API Guidelines for naming, as documented in https://rust-lang.github.io/api-guidelines/naming.html.
Declare a newtype wrapper for any branded string type being ported from TypeScript pnpm. Do not collapse the brand into a plainStringor&str; give the type its own struct so misuse is a type error.
When porting branded string types where upstream TypeScript always validates before construction, validate in the Rust port too. Construct the wrapper only viaTryFrom<String>and/orFromStr; do not provide an infallible public constructor that takes an arbitrary string.
For branded string types where upstream TypeScript never validates (used purely for type-safety to prevent confusion between string slots), expose an infallibleFrom<String>andFrom<&str>constructor in the Rust wrapper.
When upstream TypeScript occasionally constructs a branded type without validation (via bareasassertion), add afrom_str_unchecked(or similarly named) constructor on the Rust side. Keep the validating constructor as well; `from_str_u...
Files:
crates/config/src/workspace_yaml.rscrates/patching/src/hash.rscrates/patching/src/verify/tests.rs
🧠 Learnings (2)
📚 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/workspace_yaml.rscrates/patching/src/hash.rscrates/patching/src/verify/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 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/patching/src/verify/tests.rs
🔇 Additional comments (7)
crates/patching/src/verify/tests.rs (3)
8-14: Helper setup is clear and minimal.
input/entrieskeep test construction focused and reduce repeated fixture noise.
16-42: Great contract test for key iteration order.Explicitly asserting unsorted order here is the right guard against regressions in
all_patch_keys.
44-68: Coverage ofverify_patchesresult modes looks solid.The three paths (
Ok(None), warning payload, and hard error) are tested cleanly and match expected behavior.crates/patching/src/hash.rs (1)
36-45: Hashing behavior and non-UTF-8 parity look good.
from_utf8_lossy+ CRLF normalization + SHA-256 output, and the dedicated non-UTF-8 test, match the intended pnpm/Node behavior.Also applies to: 134-149
crates/config/src/workspace_yaml.rs (3)
1-13: LGTM!Imports are appropriate:
IndexMapfor order-preservingpatched_dependenciesandHashMapforallow_buildswhere lookup order doesn't matter.
71-106: LGTM!The field additions are well-designed:
IndexMapforpatched_dependenciescorrectly preserves user key order for diagnostic message parity with pnpmHashMapforallow_buildsis appropriate since lookup order doesn't affect behavior- Documentation clearly explains the deferred validation strategy and links upstream sources
211-225: LGTM!The
apply_toadditions correctly handle the new fields:
- Unconditional
workspace_dirassignment anchors path resolution for patches (matches upstream'sgetOptionsFromPnpmSettings(workspaceDir, ...))- Conditional assignments preserve Config defaults when yaml doesn't specify values
- Type handling (
Some(v)forpatched_dependenciesvs directvfor others) correctly reflects the underlying Config field types
Three follow-ups from Copilot's re-review of 9c83df9 on #425: 1. `InstallFrozenLockfile::run` was swallowing `PatchKeyConflictError` from `get_patch_info` via `if let Ok(Some(info)) ...`. Ambiguous range configs would silently skip the snapshot instead of failing with upstream's `ERR_PNPM_PATCH_KEY_CONFLICT`. Replaced the pattern with `?` propagation through a new `InstallFrozenLockfileError::PatchKeyConflict` variant — mirrors pnpm's behavior of refusing to silently pick one of the matching ranges. 2. `crates/patching/src/lib.rs` module doc still claimed nothing in the install pipeline consumed the crate. Updated to reflect slices A+B (resolve + look up per snapshot + thread into `BuildModules` cache key) and the remaining slice C work (build trigger + patch application). 3. `crates/config/src/workspace_yaml/tests.rs:196` test comment said `Config` wiring was deferred to slice B. That wiring is in this PR; updated the comment to point at `Config::resolved_patched_dependencies` directly. 566 tests pass; `just ready`, `just dylint`, `cargo doc -D warnings`, `taplo format --check` all clean. --- Written by an agent (Claude Code, claude-opus-4-7).
…ent-private-items CI's Doc job runs `cargo doc --no-deps --workspace --document-private-items` (note the last flag), which makes the private `mod get_patch_info` visible to rustdoc alongside the re-exported `pub use get_patch_info::get_patch_info`. With both visible, `[\`get_patch_info\`]` is ambiguous between a function and a module and rustdoc fails the build under `-D warnings`. The plain `cargo doc --no-deps` I'd been running locally only sees the public re-export, so the ambiguity didn't surface. Disambiguate with `()` to pin the function. Memory note for `just ready` parity updated to include `--document-private-items` in the local doc command.
Completes #397 item 9 (`patchedDependencies` support). Patches now actually land on disk: pacquet applies configured patches to extracted package directories before postinstall hooks run, includes patched-only packages in the build trigger, and surfaces the four upstream diagnostic codes. Slices A + B (PR #425, merged) landed the foundation, Config plumbing, and the side-effects-cache key wiring. This PR wires the rest. ## Pure-Rust patch applier `pacquet_patching::apply_patch_to_dir` ports upstream's [`applyPatchToDir`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/patching/apply-patch/src/index.ts) using the [`diffy`](https://crates.io/crates/diffy) crate (MIT OR Apache-2.0, pure Rust, no subprocess). Upstream's own comment notes that `patch` is unavailable on Windows and `git apply` is awkward inside an existing git repo — pnpm vendored `@pnpm/patch-package` for that reason; pacquet sidesteps it the same way with an in-process applier. Supported `FileOperation`s: `Modify`, `Create`, `Delete`. `Rename` and `Copy` fall through to `ERR_PNPM_PATCH_FAILED` with a descriptive message — they don't appear in `patch-package`-style patches in practice. ## Build pipeline - `build_sequence::get_subgraph_to_build` now considers `patch.is_some()` alongside `requires_build`. A patched-only package becomes a build candidate the same way upstream's filter at [`buildSequence.ts:47,60`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/building/during-install/src/buildSequence.ts#L40-L67) treats them. - `BuildModules::run` per-snapshot loop refactored to mirror upstream's flow: - Inner build trigger becomes `requires_build || patch.is_some()`. - `allowBuilds` check only gates scripts (upstream's `if (node.requiresBuild)` at [`during-install/src/index.ts:88-101`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/building/during-install/src/index.ts#L82-L106)). Disallow / ignored sets `should_run_scripts = false` rather than `continue` — patches still apply. - `cache_key`'s `include_dep_graph_hash` is now `should_run_scripts` (was unconditionally `true`), so a patched-only snapshot's cache key omits the deps-hash, matching upstream's `includeDepGraphHash: hasSideEffects` at [line 202](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/building/during-install/src/index.ts#L199-L204). - Patch application happens before `run_postinstall_hooks`, mirroring [`during-install/src/index.ts:171-178`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/building/during-install/src/index.ts#L168-L191). - Cache-write gate becomes `(is_patched || has_side_effects) && side_effects_cache_write` (was just `side_effects_cache_write`), matching upstream's `(isPatched || hasSideEffects)` at line 199. ## Diagnostics Four upstream codes surface via `BuildModulesError`: - `ERR_PNPM_PATCH_NOT_FOUND` — patch file missing on disk - `ERR_PNPM_INVALID_PATCH` — unified-diff parse error - `ERR_PNPM_PATCH_FAILED` — hunk wouldn't apply, target missing, IO error on a target path - `ERR_PNPM_PATCH_FILE_PATH_MISSING` — resolved patch entry has a hash but no path (lockfile-only shape with no live config); mirrors [`during-install/src/index.ts:172-176`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/building/during-install/src/index.ts#L172-L176) ## Dependency New workspace dep: `diffy = "0.5.0"`. MIT OR Apache-2.0, both in the deny.toml allow-list. `cargo deny check licenses` ok.
…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).
…5 completion) (#428) 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.
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
Combined slices A and B of #397 item 9 (
patchedDependenciessupport), plus the related #397 item 5 fix (AllowBuildPolicymoves offpackage.json).This PR adds the foundation (
pacquet-patchingcrate) and threads patches as far as the side-effects-cache key. The build-trigger update and the actual patch application are deferred to slice C — they only make sense paired together.Foundation (
pacquet-patchingcrate)Ports
@pnpm/patching.types+@pnpm/patching.config+calcPatchHashesat pnpm SHAb4f8f47ac2:types.rs) —PatchInfo,ExtendedPatchInfo,PatchGroupRangeItem,PatchGroup,PatchGroupRecordmirroringpatching/types/src/index.ts.key.rs) — minimal port ofdp.parse.group.rs) —group_patched_dependenciesmirroringgroupPatchedDependencies.ts, withERR_PNPM_PATCH_NON_SEMVER_RANGE.get_patch_info.rs) —get_patch_info(exact → unique-range → wildcard) mirroringgetPatchInfo.ts, withERR_PNPM_PATCH_KEY_CONFLICT.verify.rs) —all_patch_keys+verify_patcheswithERR_PNPM_UNUSED_PATCH.hash.rs) — SHA-256 hex with CRLF→LF normalization (matters for cross-platform stability of the cache key).resolve.rs) —resolve_and_group(workspace_dir, raw_map)resolves relative patch paths against the workspace dir, hashes, and groups.Config plumbing (workspace yaml is the source of truth)
pnpm v11 reads install settings from
pnpm-workspace.yaml, notpackage.json. This PR matches that forpatchedDependenciesand (as the #397 item 5 fix)allowBuilds/dangerouslyAllowAllBuildstoo. The misleadingly-named upstreamgetOptionsFromRootManifest.tsis called atconfig/reader/src/index.ts:814with the workspace manifest.Config.workspace_dir: Option<PathBuf>— recorded byWorkspaceSettings::apply_toso install-time code can resolve patch file paths against the same dir upstream uses.Config.patched_dependencies: Option<BTreeMap<String, String>>— raw map. Hashing + resolution deferred toConfig::resolved_patched_dependenciessoapply_tostays infallible.Config.allow_builds: HashMap<String, bool>andConfig.dangerously_allow_all_builds: bool— replaceAllowBuildPolicy::from_manifest'spackage.jsonreads.WorkspaceSettingsdeserializes the matching three yaml keys (patchedDependencies,allowBuilds,dangerouslyAllowAllBuilds);apply_topushes them.Install-pipeline threading
InstallFrozenLockfile::runcallsconfig.resolved_patched_dependencies()once per install, then for each snapshot callspacquet_patching::get_patch_info(&groups, name, version)and collects matches intoHashMap<PackageKey, ExtendedPatchInfo>keyed by the peer-stripped key. Mirrors upstream's per-resolution lookup atresolveDependencies.ts:1482, adapted for pacquet's lockfile-driven flow.BuildModulesgains apatches: Option<&HashMap<PackageKey, ExtendedPatchInfo>>field. Thecalc_dep_statecall at the existingbuild_modules.rs:326placeholder now passespatch_file_hash: patch.map(|p| p.hash.as_str()), matching upstream'spatchFileHash: depNode.patch?.hash.AllowBuildPolicy(#397 item 5)AllowBuildPolicy::from_manifest(thepackage.jsonreader was a workaround until Config plumbing landed).AllowBuildPolicy::from_config(&Config)consuming the new yaml-sourced fields.install_frozen_lockfile.rs:156switches over.What's NOT in this PR
Tracked for slice C:
requires_build || patch.is_some()) and patch application to extracted package directories before postinstall hooks. These belong together — wiring the trigger in isolation produces an incoherent state (the cache key advertises a patch that isn't on disk).git applysubprocess) will be raised when slice C opens.ERR_PNPM_PATCH_FILE_PATH_MISSING/PATCH_NOT_FOUND/INVALID_PATCH/PATCH_FAILED.simple-with-patchfixture.Test plan
groupPatchedDependencies.test.tsincludingERR_PNPM_PATCH_NON_SEMVER_RANGE.getPatchInfo.test.tsincludingERR_PNPM_PATCH_KEY_CONFLICT.shasum -a 256of"hello\n").resolve_and_groupcovers: empty input, relative path resolution, absolute path passthrough, missing patch file errors, non-semver range propagation, mixed exact/range/wildcard.WorkspaceSettingsdeserializespatchedDependencies,allowBuilds,dangerouslyAllowAllBuilds;apply_torecordsworkspace_dirand pushes each field.AllowBuildPolicy::from_configtests replace the droppedfrom_manifestcases.write_path_cache_key_includes_patch_hashrunsBuildModuleswith apatchesmap and asserts the persisted side-effects-cache row's key contains;patch=<hash>.just ready(typos, fmt, check, test, lint) clean — 565 tests pass.just dylint(perfectionist) clean.RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --workspaceclean.taplo format --checkclean.Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit