Repository navigation
feat(lockfile): emit packageExtensionsChecksum and pnpmfileChecksum for pnpm parity - #883
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 (3)
📝 WalkthroughWalkthroughImplements pnpm-compatible packageExtensions and pnpmfile checksum computation, exposes optional checksum fields on lockfile models, preserves them across graph transforms and I/O, and stamps computed checksums during install/write flows. Changespnpm Lockfile Checksum Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Greptile SummaryThis PR adds pnpm's two top-level lockfile integrity fields —
Confidence Score: 5/5Safe to merge; the change is additive and the checksum algorithms are pinned to ground-truth test vectors. The new checksum paths are well-isolated, gated on pnpm lockfile format only, and covered by round-trip + vector tests. The single finding is a sync file read inside an async function — a minor convention gap that won't affect correctness or cause observable starvation given the small, once-per-install file involved. crates/aube/src/commands/install/settings.rs — the pnpmfile_checksum sync read inside stamp_pnpm_config_checksums. Important Files Changed
Reviews (3): Last reviewed commit: "fix(lockfile): gate pnpmfileChecksum on ..." | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/aube-codes/src/warnings.rs`:
- Line 13: The constant WARN_AUBE_PNPMFILE_CHECKSUM_FAILED is declared but not
registered in the ALL array of CodeMeta entries; add a corresponding CodeMeta
entry for WARN_AUBE_PNPMFILE_CHECKSUM_FAILED to the ALL array (the same
structure used by other entries) so the metadata registry includes it for docs
and self-tests—use the constant name as the code field and fill
description/category/severity to match the style of nearby entries (ensure the
new entry follows the pattern used by other CodeMeta items and is placed with
the other warnings in the ALL array).
In `@crates/aube/src/commands/install/settings.rs`:
- Around line 495-505: The code currently preserves a stale
graph.pnpmfile_checksum when local_pnpmfile is None or when
aube_lockfile::pnpm::pnpmfile_checksum returns Err; update the control flow in
the section handling local_pnpmfile so that if let Some(path) = local_pnpmfile {
... } else { graph.pnpmfile_checksum = None; return; } and inside the match on
aube_lockfile::pnpm::pnpmfile_checksum, set graph.pnpmfile_checksum = None in
the Err(e) arm (while still emitting the tracing::warn) so missing or unreadable
pnpmfile clears the checksum instead of leaving an old value.
🪄 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: ce504bd4-2434-4102-9b92-8d929b44f10c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
crates/aube-codes/src/warnings.rscrates/aube-lockfile/Cargo.tomlcrates/aube-lockfile/src/lib.rscrates/aube-lockfile/src/pnpm/checksum.rscrates/aube-lockfile/src/pnpm/mod.rscrates/aube-lockfile/src/pnpm/raw.rscrates/aube-lockfile/src/pnpm/read.rscrates/aube-lockfile/src/pnpm/tests.rscrates/aube-lockfile/src/pnpm/write.rscrates/aube-resolver/src/peer_context.rscrates/aube-resolver/src/resolve.rscrates/aube-resolver/src/tests.rscrates/aube/src/commands/install/mod.rscrates/aube/src/commands/install/resolve.rscrates/aube/src/commands/install/settings.rs
d9c4697 to
0f2599c
Compare
|
I found one parity issue that seems worth fixing before merge:
Repro I checked with printf '{"name":"x","version":"1.0.0","dependencies":{"is-number":"7.0.0"}}\n' > package.json
: > .pnpmfile.cjs
npx pnpm@11.6.0 install --lockfile-only --ignore-scripts
rg pnpmfileChecksum pnpm-lock.yaml # no matchIf aube writes a checksum for that empty file, pnpm treats it as config drift on frozen installs: So I think this should only stamp This comment was generated by Codex. |
pnpm records pnpmfileChecksum only when a loaded pnpmfile actually exports a hooks object (requireHooks gates calculatePnpmfileChecksum on `entries.some((e) => e.hooks != null)`). aube stamped the checksum for any detected pnpmfile, so an empty or hook-less .pnpmfile.cjs grew a spurious checksum that aborts pnpm's frozen install with ERR_PNPM_LOCKFILE_CONFIG_MISMATCH. Load the pnpmfile the way pnpm does and only stamp the checksum when it exports hooks; a hook-less, unreadable, or absent pnpmfile clears it. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
Summary
pnpm writes two top-level integrity keys into
pnpm-lock.yamlthat aube was omitting, producing a gratuitous diff on everyaube installagainst a pnpm-generated lockfile:packageExtensionsChecksum— guards the effectivepackageExtensionsconfigpnpmfileChecksum— guards the active.pnpmfile.cjs/.pnpmfile.mjsThis PR computes and emits both, byte-for-byte identical to pnpm, so a workspace that uses
packageExtensionsand/or a pnpmfile round-trips cleanly between pnpm and aube. The keys are pnpm-format only;aube-lock.yamloutput is unchanged.What changed
crates/aube-lockfile/src/pnpm/checksum.rspackage_extensions_checksum: a faithful Rust port of theobject-hashnpm package (the same library pnpm uses) over the effectivepackageExtensionsmap, with pnpm's options (respectType: false,algorithm: sha256,encoding: base64, unordered arrays/objects/sets). Output is prefixedsha256-exactly as pnpm does.pnpmfile_checksum:sha256-+ base64(sha256(file content)) with\r\n→\nnormalization. Multiple pnpmfiles hash tosha256-over the joined, sorted per-file digests (pnpm's multi-file rule).LockfileGraph(crates/aube-lockfile/src/lib.rs): two new optional fields, propagated throughfilter_depsandsubset_to_importer.raw.rs,read.rs,write.rs): parse and serialize the keys. In the writable struct they sit immediately afteroverrides:and beforeimporters:, each as its own blank-line-separated scalar — matching pnpm's field order and formatting.stamp_pnpm_config_checksumshelper (commands/install/settings.rs) computes both checksums from the resolved config + on-disk pnpmfile and stamps the graph right before it's written. Wired into both write paths — the streaming install (mod.rs) and--lockfile-only(resolve.rs) — so they stay byte-identical. Gated on the target lockfile being pnpm.WARN_AUBE_PNPMFILE_CHECKSUM_FAILEDadded for the (best-effort) case where a pnpmfile can't be read for hashing.Parity verification
packageExtensionsof a production monorepo producedpackageExtensionsChecksum: sha256-abc, byte-identical to pnpm's output, plus a matchingpnpmfileChecksum.packageExtensions/pnpmfile).Notes
catalogs:afteroverrides:, whereas current pnpm emits it before. The new checksums are placed correctly relative tooverrides:in either ordering. Full byte-identical parity for workspaces that use catalogs would also need that ordering aligned — out of scope here.Test plan
cargo test -p aube-lockfile(checksum unit + round-trip tests)cargo test -p aube-resolvercargo test -p aubecargo clippy --testsclean,cargo fmt --checkcleanaube installon a pnpm workspace withpackageExtensions+ apnpmfile, then diff
pnpm-lock.yamlagainstpnpm install— checksums matchSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests