feat: foundation for the side-effects cache (#397 item #10, part 1)#422
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 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). (4)
🧰 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 (5)
📝 WalkthroughWalkthroughThis PR introduces a new ChangesGraph Hasher Library
Side Effects Cache Configuration and Integration
Sequence Diagram(s)sequenceDiagram
participant Caller
participant CalcDepState
participant CalcGraphHash
participant HashObject
participant Cache
Caller->>CalcDepState: invoke with graph and options
CalcDepState->>CalcGraphHash: compute dependency hash
CalcGraphHash->>Cache: check memoized result
alt cache hit
Cache-->>CalcGraphHash: return stored hash
else cache miss
CalcGraphHash->>HashObject: hash node object
HashObject-->>CalcGraphHash: SHA-256 digest
CalcGraphHash->>Cache: store memoized result
end
CalcGraphHash-->>CalcDepState: return hash string
CalcDepState-->>Caller: return formatted key
sequenceDiagram
participant VerifyPath
participant BuildOverlays
participant SideEffects
participant BaseMap
participant Overlay
VerifyPath->>BuildOverlays: pass side effects and base
BuildOverlays->>SideEffects: iterate cache keys
SideEffects->>BaseMap: clone for this key
BaseMap->>Overlay: apply added entries
Overlay->>Overlay: remove deleted filenames
Overlay->>Overlay: added shadows base collisions
Overlay-->>BuildOverlays: per-key FilesMap result
BuildOverlays-->>VerifyPath: return overlay map or none
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
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 |
There was a problem hiding this comment.
Pull request overview
Lays the groundwork for pnpm side-effects cache interop by adding a new hashing crate (pnpm-compatible object/dep-graph hashing), plumbing side-effects overlays through the store-dir verify path, and introducing an npmrc flag to gate cache usage.
Changes:
- Added
pacquet-graph-hashercrate implementing pnpm-compatiblehashObject+ dep-state/engine-name helpers with parity-oriented tests. - Extended store-dir verification results to surface per-cache-key overlaid
side_effects_maps. - Added
Npmrc.side_effects_cacheconfiguration knob (defaulttrue) and updated package-manager test config construction.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/store-dir/src/check_pkg_files_integrity.rs | Adds VerifyResult.side_effects_maps and builds per-cache-key overlay maps from PackageFilesIndex.side_effects. |
| crates/store-dir/src/check_pkg_files_integrity/tests.rs | Adds unit tests validating side-effects overlay behavior and None vs Some(empty) semantics. |
| crates/package-manager/src/install_package_from_registry/tests.rs | Updates Npmrc test config literal to include side_effects_cache. |
| crates/npmrc/src/lib.rs | Introduces side_effects_cache config field (default true) with documentation. |
| crates/graph-hasher/Cargo.toml | New crate manifest for pacquet-graph-hasher. |
| crates/graph-hasher/src/lib.rs | New crate root exporting object hasher, engine name, and dep-state API. |
| crates/graph-hasher/src/object_hasher.rs | Implements pnpm/object-hash compatible serialization + hashing. |
| crates/graph-hasher/src/dep_state.rs | Implements calc_dep_state / dep-graph hashing with memoization. |
| crates/graph-hasher/src/engine_name.rs | Implements pnpm ENGINE_NAME formatting and host OS/arch mapping. |
| crates/graph-hasher/src/tests.rs | Adds parity and invariants tests for hashing behavior. |
| Cargo.toml | Adds pacquet-graph-hasher to workspace dependencies. |
| Cargo.lock | Records the new workspace crate entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Micro-Benchmark ResultsLinux |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/store-dir/src/check_pkg_files_integrity/tests.rs (1)
331-338: ⚡ Quick winAdd failure context for this
assert!check.Please add a message (or
eprintln!) withresult.side_effects_mapsso failures are diagnosable without reruns.Based on learnings: "In Rust test code, follow the repo’s CODE_STYLE_GUIDE test-logging rule: add logging ... if you use assertions like
assert!,assert_ne!, etc."🤖 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/store-dir/src/check_pkg_files_integrity/tests.rs` around lines 331 - 338, The test no_side_effects_yields_none currently uses assert! without context; modify the assertion to include diagnostic output by either using assert!(result.side_effects_maps.is_none(), "side_effects_maps = {:?}", result.side_effects_maps) or printing the value before the assertion (e.g., eprintln!("side_effects_maps = {:?}", result.side_effects_maps)) so failures show the actual result; update the assertion surrounding result.side_effects_maps in that test accordingly.
🤖 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/graph-hasher/src/engine_name.rs`:
- Around line 50-57: The host_arch() function currently returns Rust's
std::env::consts::ARCH values directly for unknown architectures, which leaks
Rust-only identifiers (e.g., powerpc64le, loongarch64) into ENGINE_NAME and
breaks pnpm cache-key parity; update host_arch() (the match in function
host_arch) to explicitly map "powerpc64le" => "ppc64" and "loongarch64" =>
"loong64" alongside the existing arms so ENGINE_NAME uses Node.js-style arch
names while keeping the existing mappings for "x86_64", "aarch64", "x86", and
the fallback other => other.
In `@crates/npmrc/src/lib.rs`:
- Around line 196-211: The documentation for the side_effects_cache option
overstates behavior by claiming it skips lifecycle scripts and forces rebuilds,
but the current implementation only implements the read-path (consulting
PackageFilesIndex.sideEffects keyed by pacquet_graph_hasher::engine_name plus
dep-graph/patch hashes) and does not gate skipping builds or lifecycle scripts;
update the comment block that documents side_effects_cache (and any mentions of
sideEffectsCache) to explicitly describe only the implemented read-path
behavior: that a matching cache entry will be used as the import payload
(overlay applied to the base CAFS map) when present, and remove or reword any
language asserting that lifecycle scripts are skipped or rebuilds are prevented.
---
Nitpick comments:
In `@crates/store-dir/src/check_pkg_files_integrity/tests.rs`:
- Around line 331-338: The test no_side_effects_yields_none currently uses
assert! without context; modify the assertion to include diagnostic output by
either using assert!(result.side_effects_maps.is_none(), "side_effects_maps =
{:?}", result.side_effects_maps) or printing the value before the assertion
(e.g., eprintln!("side_effects_maps = {:?}", result.side_effects_maps)) so
failures show the actual result; update the assertion surrounding
result.side_effects_maps in that test accordingly.
🪄 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: 03ad0f96-a736-4432-83da-167e019292c3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.tomlcrates/graph-hasher/Cargo.tomlcrates/graph-hasher/src/dep_state.rscrates/graph-hasher/src/engine_name.rscrates/graph-hasher/src/lib.rscrates/graph-hasher/src/object_hasher.rscrates/graph-hasher/src/tests.rscrates/npmrc/src/lib.rscrates/package-manager/src/install_package_from_registry/tests.rscrates/store-dir/src/check_pkg_files_integrity.rscrates/store-dir/src/check_pkg_files_integrity/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). (6)
- GitHub Check: Agent
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Log emissions are part of 'match pnpm'—when porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(...), orstreamParser.write(...), mirror the call site, payload, and ordering so@pnpm/cli.default-reporterparses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing into plainStringor&strto preserve the distinction between validating and non-validating brands
If upstream always validates before construction of a branded string type, implement validation viaTryFrom<String>and/orFromStrwithout providing an infallible public constructor
If upstream never validates a branded string type, expose an infallibleFrom<String>constructor to provide type-safety without runtime validation
When upstream occasionally constructs a branded string type without validation via bareasassertion, expose afrom_str_uncheckedconstructor so callers can opt into the same unchecked path explicitly
Match upstream serde behavior for branded types—use#[serde(try_from = "String")]for deserialization to go through the validator and#[serde(into = "String")]for serialization
Use#[derive(derive_more::From)]and#[derive(derive_more::Into)]for mechanical conversion impls on branded types rather than handwritingimplblocks
Model string-literal unions (like'auto' | 'always' | 'never') as Rustenums instead of newtype wrappers
Template literal types (like${string}@${string}) should be treated as branded string types with the same validation discipline applied
Follow the code style guide inCODE_STYLE_GUIDE.mdcovering code-level conventions not enforced by tooling: imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching,pipe-trait, error handling, test layout, and cloning ofArcandRc
Warnings are errors incargo clippy --locked -- --deny warnings, and should not be silenced with `#[allow(....
Files:
crates/graph-hasher/src/engine_name.rscrates/graph-hasher/src/lib.rscrates/npmrc/src/lib.rscrates/package-manager/src/install_package_from_registry/tests.rscrates/store-dir/src/check_pkg_files_integrity/tests.rscrates/graph-hasher/src/tests.rscrates/graph-hasher/src/dep_state.rscrates/store-dir/src/check_pkg_files_integrity.rscrates/graph-hasher/src/object_hasher.rs
🧠 Learnings (3)
📚 Learning: 2026-05-07T23:19:08.272Z
Learnt from: KSXGitHub
Repo: pnpm/pacquet PR: 401
File: tasks/integrated-benchmark/src/work_env.rs:343-344
Timestamp: 2026-05-07T23:19:08.272Z
Learning: When reviewing Rust code in pnpm/pacquet for deprecated API usage, do not automatically treat `serde_saphyr::to_string` as deprecated. In `serde-saphyr` v0.0.25, `serde_saphyr::to_string` has no `#[deprecated]` attribute (the `#[deprecated]` later in `serde-saphyr-0.0.25/src/lib.rs` applies to a different function). Only flag `serde_saphyr::to_string` as deprecated if the resolved dependency version’s source shows `#[deprecated]` on that specific function.
Applied to files:
crates/graph-hasher/src/engine_name.rscrates/graph-hasher/src/lib.rscrates/npmrc/src/lib.rscrates/package-manager/src/install_package_from_registry/tests.rscrates/store-dir/src/check_pkg_files_integrity/tests.rscrates/graph-hasher/src/tests.rscrates/graph-hasher/src/dep_state.rscrates/store-dir/src/check_pkg_files_integrity.rscrates/graph-hasher/src/object_hasher.rs
📚 Learning: 2026-05-01T10:01:33.766Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 349
File: crates/reporter/src/tests.rs:121-121
Timestamp: 2026-05-01T10:01:33.766Z
Learning: In Rust test code, follow the repo’s CODE_STYLE_GUIDE test-logging rule: add logging (e.g., `eprintln!`/`eprintln!(...)`) so that useful diagnostic values are printed when a test fails, unless the assertion is `assert_eq!` (where the differing values are already included). Concretely, if you use assertions like `assert!`, `assert_ne!`, etc., ensure the test logs the relevant actual/expected values (or context) before/around the assertion so failures can be diagnosed without rerunning.
Applied to files:
crates/package-manager/src/install_package_from_registry/tests.rscrates/store-dir/src/check_pkg_files_integrity/tests.rscrates/graph-hasher/src/tests.rs
📚 Learning: 2026-05-01T10:01:33.766Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 349
File: crates/reporter/src/tests.rs:121-121
Timestamp: 2026-05-01T10:01:33.766Z
Learning: In this repo’s Rust tests (run via `cargo nextest`), stdout/stderr from passing tests is captured and is only shown when a test fails. As a result, `eprintln!` (and similar stderr logging) in these tests should generally not be flagged as CI “noise” on the happy path; CI output should only appear for failing tests.
Applied to files:
crates/graph-hasher/src/tests.rs
🔇 Additional comments (4)
crates/package-manager/src/install_package_from_registry/tests.rs (1)
39-39: Nice explicit fixture pin.Setting
side_effects_cache: truehere keeps this test isolated from future default changes inNpmrc.crates/store-dir/src/check_pkg_files_integrity.rs (1)
214-248: Overlay map construction looks solid.The per-cache-key merge (
addedprecedence +deletedfiltering + base fallback) is implemented cleanly and wired consistently into both verify paths.crates/store-dir/src/check_pkg_files_integrity/tests.rs (1)
340-456: Great coverage additions for side-effects overlays.These tests pin the important semantics (add/delete merge, collision precedence, and per-key isolation) well.
crates/graph-hasher/src/object_hasher.rs (1)
107-111: ⚡ Quick winSort keys with UTF-16 ordering if needed for pnpm parity, but fix the proposed syntax.
The proposed diff has a compilation error:
encode_utf16()returns an iterator withoutOrd, so it cannot be directly compared with.cmp(). If this change is necessary, use:keys.sort_by(|a, b| { a.encode_utf16().collect::<Vec<u16>>() .cmp(&b.encode_utf16().collect::<Vec<u16>>()) })However, the practical necessity is unclear: all object keys in tests are ASCII-only, and real-world package manager keys follow npm naming rules (ASCII). Confirm whether upstream's object-hash actually uses plain
Array.prototype.sortfor object keys and whether non-BMP ordering differences occur in practice before implementing.
Doc CI blocker + five Copilot/coderabbit threads: - `npmrc` referenced `[pacquet_graph_hasher::engine_name]` from a crate that doesn't depend on graph-hasher, so rustdoc rejected the unresolvable link. Also flagged: a stale `[pacquet_platform](#)` placeholder link in `graph-hasher::engine_name`'s docstring, and a `[serialize]` link from a public doc to a private helper in `object_hasher`. Reworded all three to plain prose so docs build under `-D warnings`. - Reworded the `Npmrc.side_effects_cache` doc to describe **only the foundation-on-the-wire** state this PR actually ships (no rebuild-skip gate, no WRITE path). The previous wording asserted behavior that lands in #421. - `object_hasher` factored a `serialize_str` helper out of the string arm so `write_pair` no longer allocates a per-key `Value::String(key.to_string())`. Hot path on hashing large graphs; the extra `Number::to_string()` divergence question is noted in a clarifying doc comment on the `Value::Number` arm (pacquet only ever hashes integer values; non-integer floats would need a JS-shortest-round-trippable formatter — `ryu` or similar — before they can land safely on the cache-key path). - `build_side_effects_maps` was O(|base| * |deleted|) — every base entry called `deleted_set.iter().any(...)`. Promote `deleted` to a `HashSet` once per cache key for linear time, matching what pnpm's TS side does with a `Set`. - `engine_name::host_arch` was passing `powerpc64le` / `loongarch64` through verbatim, which would have leaked Rust-only identifiers into `ENGINE_NAME` and broken cache-key parity on those architectures. Added explicit mappings: `powerpc64le → ppc64` (Node uses the same string for BE and LE POWER; only `os.endianness()` distinguishes them) and `loongarch64 → loong64`.
New `pacquet-graph-hasher` crate porting: - `@pnpm/crypto.object-hasher` (`hashObject` + `hashObjectWithoutSorting`) from <https://github.com/pnpm/pnpm/blob/b4f8f47ac2/crypto/object-hasher/src/index.ts>. Replicates the `object-hash@3.0.0` bytestream format (`object:<N>:<key>:<value>,...` with sorted keys when `unorderedObjects`, `string:<utf16_len>:<value>`, etc.) plus sha256 + base64/hex encoding. Byte-for-byte parity with pnpm is load-bearing — the cache key is persisted on disk and shared with pnpm. The known-base64 test pins `hashObject({b:1,a:2}) == "48AVoXIXcTKcnHt8qVKp5vNw4gyOB5VfztHwtYBRcAQ="`. - `@pnpm/deps.graph-hasher`'s `calcDepState` / `calcDepGraphHash` from <https://github.com/pnpm/pnpm/blob/b4f8f47ac2/deps/graph-hasher/src/index.ts>. Recursive walk over a `DepsGraph` (alias → child key) memoized via `DepsStateCache`. Cycle-safe via a `parents` set; mirrors upstream's `if (!parents.has(node.fullPkgId))` short-circuit. - `ENGINE_NAME` from `core/constants/src/index.ts:7` — `<platform>;<arch>;node<major>` with Node-naming mapping for the host (`darwin` / `win32` / `sunos`; `x64` / `arm64` / `ia32` / `ppc64`). Tests: 8 hashObject parity cases (including the byte-exact pnpm fixture, sorted vs unsorted, hex/base64, UTF-16 length, dep-state shape) + 6 calcDepState cases (engine-only, patch-only, leaf, diamond, cycle, ordering) + 2 engine_name cases (format + host default shape). Scoped narrowly to what pacquet's side-effects cache READ path actually feeds in (strings, objects of strings, null, booleans, numbers). Set / Map / Date / Buffer / Array unordered-permutation arms from upstream are unimplemented — they have no caller in pacquet's tree today. Adding them can land alongside a future caller that exercises them.
Pacquet's `PackageFilesIndex.side_effects` field has been on the wire round-trip since the schema landed but the verify path dropped it — `check_pkg_files_integrity` and `build_file_maps_from_index` discarded everything except `files` and `algo`, so callers couldn't tell whether a stored package had a side-effects cache populated. `VerifyResult` now carries `side_effects_maps: Option<HashMap<String, FilesMap>>`. Each entry is the post-build files map for one cache key — built by applying the entry's `SideEffectsDiff.added` overlay on top of the base files_map and dropping any filenames in `deleted`. Mirrors `applySideEffectsDiffWithMaps` at <https://github.com/pnpm/pnpm/blob/b4f8f47ac2/store/create-cafs-store/src/index.ts#L103-L121>. Three test cases cover the overlay's invariants: `added` wins on filename collision, `deleted` drops base entries, and multiple cache keys produce independent overlays. The fourth pins the "no side-effects field on the row" path to return `None` so the importer can distinguish "cache never written" from "cache empty for the targeted key". `None` for the `added` overlay's CAFS-path resolution drops that single filename (with a debug log) instead of failing the whole overlay — matches upstream's lenient `getFlatMap` which would likewise surface the missing-CAS-blob error later at import time, not during verify.
Pacquet `Npmrc` gains `side_effects_cache: bool` (default `true`, matching pnpm). Mirrors pnpm's [`side-effects-cache`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/config/config/src/index.ts) config (camelCase `sideEffectsCache`). When `true`, the build phase will consult `PackageFilesIndex.side_effects` to decide whether a package is already built and skip its lifecycle scripts. When `false`, the cache is ignored and lifecycle scripts run unconditionally. The read-side cache lookup itself lands with the `is_built` wiring in a follow-up commit; the write side (populating the cache after a postinstall) is a separate PR.
… fixtures (#397) `create_config` in install_package_from_registry's tests builds an `Npmrc` literal that was missing the field added in the previous commit. Default-on to match production.
Doc CI blocker + five Copilot/coderabbit threads: - `npmrc` referenced `[pacquet_graph_hasher::engine_name]` from a crate that doesn't depend on graph-hasher, so rustdoc rejected the unresolvable link. Also flagged: a stale `[pacquet_platform](#)` placeholder link in `graph-hasher::engine_name`'s docstring, and a `[serialize]` link from a public doc to a private helper in `object_hasher`. Reworded all three to plain prose so docs build under `-D warnings`. - Reworded the `Npmrc.side_effects_cache` doc to describe **only the foundation-on-the-wire** state this PR actually ships (no rebuild-skip gate, no WRITE path). The previous wording asserted behavior that lands in #421. - `object_hasher` factored a `serialize_str` helper out of the string arm so `write_pair` no longer allocates a per-key `Value::String(key.to_string())`. Hot path on hashing large graphs; the extra `Number::to_string()` divergence question is noted in a clarifying doc comment on the `Value::Number` arm (pacquet only ever hashes integer values; non-integer floats would need a JS-shortest-round-trippable formatter — `ryu` or similar — before they can land safely on the cache-key path). - `build_side_effects_maps` was O(|base| * |deleted|) — every base entry called `deleted_set.iter().any(...)`. Promote `deleted` to a `HashSet` once per cache key for linear time, matching what pnpm's TS side does with a `Set`. - `engine_name::host_arch` was passing `powerpc64le` / `loongarch64` through verbatim, which would have leaked Rust-only identifiers into `ENGINE_NAME` and broken cache-key parity on those architectures. Added explicit mappings: `powerpc64le → ppc64` (Node uses the same string for BE and LE POWER; only `os.endianness()` distinguishes them) and `loongarch64 → loong64`.
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.36394728846,
"stddev": 0.09138562609097754,
"median": 2.3791960675599997,
"user": 2.5093817599999997,
"system": 3.26813546,
"min": 2.24237128656,
"max": 2.50397130356,
"times": [
2.36019679656,
2.41070823656,
2.50397130356,
2.24237128656,
2.42030176656,
2.46511043256,
2.32063816056,
2.39819533856,
2.25253542356,
2.26544413956
]
},
{
"command": "pacquet@main",
"mean": 2.3078971159600004,
"stddev": 0.027644467822877203,
"median": 2.3122525940600003,
"user": 2.53633296,
"system": 3.250820859999999,
"min": 2.26198936156,
"max": 2.35358159356,
"times": [
2.2638349085600002,
2.35358159356,
2.32044115256,
2.30212641056,
2.32489312556,
2.30517505856,
2.32242436056,
2.26198936156,
2.31398014056,
2.31052504756
]
},
{
"command": "pnpm",
"mean": 5.7301268834600005,
"stddev": 0.06627787375035617,
"median": 5.7329107835599995,
"user": 8.35900466,
"system": 4.23655966,
"min": 5.63949244056,
"max": 5.81878152756,
"times": [
5.77817834156,
5.65262863156,
5.69456559656,
5.81878152756,
5.80150765856,
5.6672904195600005,
5.63949244056,
5.76473884256,
5.70108272456,
5.78300265156
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.70189347774,
"stddev": 0.14586133810676244,
"median": 0.66182034264,
"user": 0.33230544,
"system": 1.47272578,
"min": 0.6358420756400001,
"max": 1.1133423386399999,
"times": [
1.1133423386399999,
0.6619665006400001,
0.66167418464,
0.63689118864,
0.6632626196400001,
0.63885713164,
0.6360577016400001,
0.68035607464,
0.69068496164,
0.6358420756400001
]
},
{
"command": "pacquet@main",
"mean": 0.6509423267400001,
"stddev": 0.03514030011057033,
"median": 0.6365340651400001,
"user": 0.33397534,
"system": 1.4705931799999996,
"min": 0.6251678216400001,
"max": 0.72377757064,
"times": [
0.72377757064,
0.63089589264,
0.64930018164,
0.6333950166400001,
0.6349134566400001,
0.63906853064,
0.7081813036400001,
0.6381546736400001,
0.6251678216400001,
0.6265688196400001
]
},
{
"command": "pnpm",
"mean": 2.4319712743400004,
"stddev": 0.10076581792743512,
"median": 2.39829122264,
"user": 2.8207817399999997,
"system": 2.1747805799999997,
"min": 2.31379282364,
"max": 2.5731766246400003,
"times": [
2.5731766246400003,
2.34874307864,
2.4059903626400003,
2.5422821136400002,
2.3571400436400003,
2.31379282364,
2.39059208264,
2.32757182964,
2.54007594764,
2.52034783664
]
}
]
} |
…base The rebase onto main pulled in #420's rename of `pacquet-npmrc` → `pacquet-config`, which dropped the ini-wiring attributes (`#[serde(default = "bool_true", deserialize_with = "deserialize_bool")]`) from every field. My original commit on the now-renamed file carried those attributes along; once the helpers are gone they fail to compile (`cannot find attribute serde in this scope`). Strip them so the field matches the post-rename style — only the `#[default = true]` from `smart-default` remains, which is what the rest of the struct uses.
238c9cd to
96a2104
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #422 +/- ##
==========================================
+ Coverage 85.23% 85.72% +0.49%
==========================================
Files 79 82 +3
Lines 5372 5704 +332
==========================================
+ Hits 4579 4890 +311
- Misses 793 814 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
`crate::engine_name` is ambiguous to rustdoc — the name resolves to both a function (the public `engine_name()` re-export) and a module (the private `mod engine_name`). Use the explicit function-call form `[\`crate::engine_name()\`]` so rustdoc dispatches to the fn. Caught by the workspace-wide `RUSTDOCFLAGS=-D warnings` CI check.
Two Copilot threads:
- `side_effects_cache` was on the `Config` struct but unreachable
from any config source — `WorkspaceSettings` had no
`sideEffectsCache` field and `apply_to` didn't set it. So a
user could only flip it via direct `Config { … }` construction.
Added `WorkspaceSettings.side_effects_cache: Option<bool>` and
threaded it through `apply_to`'s macro list. Now
`sideEffectsCache: false` in `pnpm-workspace.yaml` takes effect.
Tightened the field's docstring to describe what's actually
wired (yaml-source plumbing on, build-phase gate still in
#421). New test mirrors the existing
`parses_verify_store_integrity_from_yaml_and_applies` shape:
pin both the camelCase rename and the `apply_to` step.
- `build_side_effects_maps` previously continued building an
overlay when a single `added` entry had a malformed digest —
it logged + skipped just that file and kept going. Once the
importer in #421 starts flipping `is_built = true`
on overlay presence, that partial overlay becomes a silent
corruption (build skipped, required artifact missing). Switch
to `continue 'next_key` so the entire cache_key entry is
dropped, sending the package back through the rebuild path.
Other cache_key entries on the same package survive. New
test pins the fail-closed behavior.
…path (#421 slice A) (#423) ## Summary Implements slice (A) of #421 — the READ-path gate that consumes the foundation merged in #422. With a matching side-effects-cache entry on disk (typically seeded by a prior `pnpm install`), `BuildModules` now skips lifecycle scripts for that snapshot, mirroring upstream's [`!node.isBuilt` filter](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/building/during-install/src/index.ts#L73-L77). User-visible behavior: a warm `pacquet install` after `pnpm install` no longer rebuilds packages whose post-build state is already in the store. ## What lands - **`pacquet_graph_hasher::detect_node_major()`** — spawns `node --version` once at install start, parses the major version. The parse helper is factored out so the rule is unit-tested without a child process. Missing-node returns `None` and the gate falls through to "rebuild" (safe). - **`DepsGraphNode` flipped to own its strings** — was borrowed `&'a str`, now `String`. Lets callers building the graph from a lockfile skip a separate-arena lifetime story. No external consumers besides in-crate tests yet, so this is internal-only. - **`PrefetchResult.side_effects_maps`** surfaces the per-row `cache_key → FilesMap` overlay that the prefetch loop previously dropped (since #422 added it to `VerifyResult`). `Arc`-wrapped for the same cold-batch cheap-clone reason `cas_paths` is. - **`CreateVirtualStore` returns a new `CreateVirtualStoreOutput`** struct: `package_manifests` (existing) plus `side_effects_maps_by_snapshot: HashMap<PackageKey, Arc<…>>` (new). Peer-variants of the same package share one `Arc<…>` because the store-index row is keyed peer-stripped. - **New `pacquet_package_manager::deps_graph::build_deps_graph` adapter** — walks `snapshots` + `packages` and builds a `DepsGraph<PackageKey>` for the hasher. `full_pkg_id` derives from `<pkg_id>:<integrity>` for registry / tarball resolutions, or `<pkg_id>:<hashObject(resolution)>` for git / directory / integrity-less tarball, matching upstream's [`createFullPkgId`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/deps/graph-hasher/src/index.ts#L263-L292). - **`BuildModules` is_built gate** — after the policy gate, before extraction, the build loop computes `calc_dep_state` per snapshot (with `include_dep_graph_hash: true` mirroring upstream's `hasSideEffects`), looks the cache key up in the threaded-in `side_effects_maps`, and `continue`s on hit. The `Config.side_effects_cache: false` flag short-circuits the lookup entirely. - **`InstallFrozenLockfile`** detects the engine name once and threads it (plus the prefetch's side-effects map and the config flag) into `BuildModules`. ## Tests - `using_side_effects_cache_skips_rebuild` ports the upstream `'using side effects cache'` case from [`installing/deps-installer/test/install/sideEffects.ts:79`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/installing/deps-installer/test/install/sideEffects.ts#L79-L131). Hand-builds the `side_effects_maps_by_snapshot` entry with the exact cache key `BuildModules` will compute, asserts no `pnpm:lifecycle` event fires for `@pnpm.e2e/failing-postinstall`'s postinstall (i.e. the build was skipped — if the gate were broken the script would have run and the install would have failed with `LifecycleScript(_)`). - `side_effects_cache_disabled_bypasses_the_gate` (negative pair): same overlay shape with `side_effects_cache: false`, expects the build to run and the install to fail with the underlying `exit 1` from the postinstall. - `deps_graph::tests`: registry-resolution uses integrity verbatim, dependencies + optionalDependencies fold into children, snapshot without metadata is skipped. The upstream test runs the install twice — first to populate the cache via the WRITE path, then to consume it. Pacquet doesn't have a WRITE path yet (#421 slice B), so the port hand-crafts the same in-memory state directly. The behavior under test is identical. ## Out of scope - **WRITE path** — pacquet seeding the cache itself (re-CAFS post-build, calc diff, mutate index row). Tracked as slice (B) in #421. - **Patch-file hash in the cache key** — depends on `patchedDependencies` (#397 item 9).
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
Foundation for porting pnpm's side-effects cache (item #10 of #397). This PR lands the three pieces that the actual cache read/write paths depend on:
pacquet-graph-hashercrate porting pnpm's@pnpm/crypto.object-hasher(hashObject/hashObjectWithoutSorting) plus@pnpm/deps.graph-hasher(calcDepState/calcDepGraphHash) andENGINE_NAME. Byte-for-byte parity with the JSobject-hash@3.0.0bytestream format is load-bearing — the cache key is persisted on disk and shared with pnpm. The headline parity test pinshashObject({b:1,a:2}) == "48AVoXIXcTKcnHt8qVKp5vNw4gyOB5VfztHwtYBRcAQ="against the upstream fixture incrypto/object-hasher/test/index.ts:6.VerifyResult.side_effects_mapsinpacquet-store-dir. The verify path (check_pkg_files_integrityandbuild_file_maps_from_index) used to dropPackageFilesIndex.side_effectsafter extraction. It now applies theadded/deletedoverlay per cache key and surfaces aHashMap<cache_key, FilesMap>— the same shape pnpm uses for itsPackageFilesResponse.sideEffectsMaps. MirrorsapplySideEffectsDiffWithMaps.Config.side_effects_cacheconfig knob (defaulttrue, matching pnpm). Surfaced on the wire only — no consumer yet. Wires up cleanly once the build-phase gate lands in Wire is_built skip + side-effects cache WRITE path (#397 item #10 follow-up to PR #422) #421, with no config migration needed for downstream callers.Scoped narrowly
This is only the foundation. There is no
is_builtfield, noBuildModulesskip gate, and no end-to-end test of the rebuild-skip behavior. Those land in a separate follow-up tracked in #421, which also covers the WRITE path (pacquet seeding the cache itself).The natural slice for review: graph-hasher unblocks both READ and WRITE, the verify-path surfacing is the only intrusive cross-crate change, and the config knob is forward-looking.
Hashing scope, explicitly
The
pacquet-graph-hasherport implements only the type arms pacquet actually feeds intohashObjectfor the cache-key path: strings, objects, numbers, booleans, null, and arrays in their ordered form.Set/Map/Date/Buffer/Arrayunordered-permutation arms from upstreamobject-hashare unimplemented — no caller in pacquet's tree feeds them in today, and the test fixtures upstream uses (hashObject({ b: new Set([…]), a: [...] })) hash deterministically across orderings only because they're testing the library, not exercising a real call path. Adding them can land alongside a future caller that needs them.Test plan
just ready(494 tests + clippy clean)taplo format --check(clean —just readydoesn't run this, CI does)Next
is_builtskip + cache WRITE path.Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
New Features
side_effects_cacheconfiguration (default: enabled) and per-cache-key side-effects overlays in package integrity results.Chores
Tests