Repository navigation
fix(install): close pnpm-lock.yaml parity and re-resolution gaps - #896
Merged
Merged
Conversation
propagate_peer_suffixes_to_ancestors lifted a descendant's (peer@version) suffix onto every non-peer-declaring ancestor, even when an ancestor supplied that peer itself. A transitively-needed optional peer like fdir's picomatch then leaked all the way up the chain (tinyglobby, node-gyp, node-expat, xml2json), so aube wrote xml2json@0.12.0(picomatch@4.0.4) where pnpm writes xml2json@0.12.0. Suppress a peer once it reaches a package that supplies it (in its own dependencies or optionalDependencies): pnpm resolves the peer there and stops the suffix, leaving the supplier and everything above it bare. The peer *declarer* (fdir) keeps its suffix via the existing has_own_peers gate. Co-authored-by: Cursor <cursoragent@cursor.com>
pnpm records a github/gitlab/bitbucket dep pinned to a 40-char SHA as a
codeload `RemoteTarball` (`name@https://codeload…/tar.gz/<sha>` with
`resolution: {gitHosted, integrity, tarball}` + `version:`), not the
`<url>.git#<sha>` / `type: git` form aube emitted. The resolver already
fetched the codeload tarball; promote the resolved `LocalSource::Git` to
`RemoteTarball` whenever a flat HTTPS archive URL exists and there's no
`&path:` subpath, then emit `version:` for remote-tarball entries even
on fresh resolve (their dep_path is the hashed `url+<hash>` form, so the
`url_keyed` heuristic was false). Also sort `resolution:` keys
alphabetically to match pnpm's sorted-key emitter.
Co-authored-by: Cursor <cursoragent@cursor.com>
Only the main install path stamped packageExtensionsChecksum and pnpmfileChecksum into the lockfile; update, remove, dedupe and audit --fix resolved a fresh graph (checksums None) and wrote it verbatim, dropping both fields from pnpm-lock.yaml. Add install::finalize_lockfile_graph — detect lockfile kind, load the resolve context + local pnpmfile, stamp checksums, then run the existing prepare pass — and route the four commands through it before writing. The shared-workspace merge path threads the same --ignore-pnpmfile / --pnpmfile flags the member resolve used so the root stamp can't re-add a pnpmfileChecksum the user opted out of. Co-authored-by: Cursor <cursoragent@cursor.com>
The warm-path settings hash folded in the workspace YAML (so a
pnpmfilePath *setting* change busted it) but never the pnpmfile's own
bytes. Editing a local .pnpmfile.{cjs,mjs} readPackage/afterAllResolved
hook therefore rode the warm path: the hook silently never re-applied,
leaving node_modules and the lockfile stale and suppressing pnpm's
readPackage output. Hash the resolved pnpmfile's path + content into the
settings hash so a changed hook forces a full re-resolve, mirroring
pnpm folding the pnpmfile into its own up-to-date check.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR introduces ChangesHosted-git tarball representation and peer-suffix round-trip
Content-affected dep-path detection for prewarm deferral
finalize_lockfile_graph centralization and pnpmfile checksums
Sequence Diagram(s)sequenceDiagram
participant Cmd as audit/dedupe/<br/>remove/update
participant Finalize as finalize_lockfile_graph
participant Settings as install/settings
participant ResolveCtx as ResolveCtx<br/>(workspace)
participant Write as pnpm/write
participant Read as pnpm/read
Cmd->>Finalize: (cwd, &mut graph, manifest,<br/>ignore_pnpmfile, cli_pnpmfile)
Finalize->>Settings: detect kind, load workspace,<br/>find pnpmfile
Settings-->>Finalize: ResolveCtx + pnpmfile path
Finalize->>Finalize: stamp package_extensions_checksum<br/>+ pnpmfile checksum
Finalize->>Finalize: prepare_resolved_graph_for_lockfile_write
Cmd->>Write: write lockfile
Write->>Write: peer_suffix_to_spec:<br/>hashed → spec form
Write-->>Cmd: written YAML<br/>(spec-form suffixes)
Cmd->>Read: reparse lockfile
Read->>Read: detect git/tarball locals,<br/>build spec→hash map
Read->>Read: rewrite_peer_suffix:<br/>spec → hashed form
Read-->>Cmd: normalized LockfileGraph<br/>(hashed keys)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~110 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
A registry package that peers with a git or remote-tarball dep wrote the suffix as aube's internal FS-safe hashed dep_path (`request-promise-core@1.1.4(request@git+<hash>)`); pnpm writes the resolved spec (`...(request@https://codeload.github.com/.../tar.gz/<sha>)`). Translate hashed->spec on write and spec->hashed on read so the lockfile matches pnpm and a round-trip stays a fixed point (a one-sided change would re-key every install and churn the lockfile). Co-authored-by: Cursor <cursoragent@cursor.com>
rubnogueira
force-pushed
the
fix/pnpmfile-and-lock-compat
branch
from
June 14, 2026 22:38
9ff7fa2 to
13b9db7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/aube-lockfile/src/pnpm/write.rs (1)
86-92:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve peer suffixes when serializing contextualized git/tarball deps.
These
local.specifier()fast paths drop the(...)suffix thatapply_peer_contextsput on the dep path. ForLocalSource::Git/LocalSource::RemoteTarball, that means the importerversion, snapshot dependency value, and snapshot key all collapse to the bare URL, so distinct peer variants can overwrite each other and reparsing can’t reconstruct the original graph. The read side already strips URL-based peer suffixes during classification, so the writer needs to add them back here for parity.Also applies to: 529-533, 555-558
🤖 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/aube-lockfile/src/pnpm/write.rs` around lines 86 - 92, The local.specifier() calls for Git and RemoteTarball sources are dropping the peer context suffix (the `(...)` part) from the dep_path, which causes distinct peer variants to collapse into the same URL and prevents proper reparsing of the graph. You need to preserve and include the peer suffix when serializing these contextualized dependencies. Check the dep.dep_path to see if it contains a peer context suffix and append it back to the result of local.specifier() for LocalSource::Git and LocalSource::RemoteTarball variants at the anchor location (lines 86-92) and at the sibling locations mentioned in the consolidated_sites (lines 529-533 and 555-558). Ensure the suffix is reconstructed consistently across all three locations so the importer version, snapshot dependency value, and snapshot key maintain the peer context information.
🤖 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/src/commands/install/settings.rs`:
- Around line 554-574: The `finalize_lockfile_graph` function at line 564
silently ignores errors from `aube_manifest::workspace::load_both(cwd)` by using
`unwrap_or_default()`, which masks malformed pnpm-workspace.yaml files and
causes incorrect checksum stamping. Change the return type of
`finalize_lockfile_graph` from unit type to a Result type to propagate errors
properly. Replace the `unwrap_or_default()` call on `load_both()` with the `?`
operator to fail fast on invalid workspace configuration, matching the install
command's error-handling behavior. Update all callers of
`finalize_lockfile_graph` to handle the Result return type appropriately.
---
Outside diff comments:
In `@crates/aube-lockfile/src/pnpm/write.rs`:
- Around line 86-92: The local.specifier() calls for Git and RemoteTarball
sources are dropping the peer context suffix (the `(...)` part) from the
dep_path, which causes distinct peer variants to collapse into the same URL and
prevents proper reparsing of the graph. You need to preserve and include the
peer suffix when serializing these contextualized dependencies. Check the
dep.dep_path to see if it contains a peer context suffix and append it back to
the result of local.specifier() for LocalSource::Git and
LocalSource::RemoteTarball variants at the anchor location (lines 86-92) and at
the sibling locations mentioned in the consolidated_sites (lines 529-533 and
555-558). Ensure the suffix is reconstructed consistently across all three
locations so the importer version, snapshot dependency value, and snapshot key
maintain the peer context information.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 74f6e6c2-11b8-4f7a-bbbf-201db5ca9b23
📒 Files selected for processing (14)
crates/aube-lockfile/src/pnpm/dep_path.rscrates/aube-lockfile/src/pnpm/read.rscrates/aube-lockfile/src/pnpm/tests.rscrates/aube-lockfile/src/pnpm/write.rscrates/aube-resolver/src/local_source.rscrates/aube-resolver/src/peer_context.rscrates/aube-resolver/src/tests.rscrates/aube/src/commands/audit.rscrates/aube/src/commands/dedupe.rscrates/aube/src/commands/install/mod.rscrates/aube/src/commands/install/settings.rscrates/aube/src/commands/remove.rscrates/aube/src/commands/update.rscrates/aube/src/state.rs
Contributor
Greptile SummaryThis PR fixes a cluster of
Confidence Score: 5/5Safe to merge — well-scoped bug fixes with extensive regression tests; the one non-blocking concern in hosted_git_local_source is a theoretical edge case both call sites already prevent in practice. All five bug fixes are orthogonal, each backed by new unit and integration tests. The peer-suffix rewrite is symmetric (writer to spec, reader to hashed) so round-trips are fixed-points. The finalize_lockfile_graph refactor introduces one additional failure mode — a malformed pnpm-workspace.yaml now fails commands that previously silently continued — but this is intentional and documented. The only non-blocking concern is integrity.unwrap_or_default() in hosted_git_local_source, where both call sites already guarantee a non-None integrity when a codeload URL is present. crates/aube-resolver/src/local_source.rs — the integrity.unwrap_or_default() in hosted_git_local_source warrants a follow-up to make the non-None invariant explicit. Important Files Changed
Reviews (4): Last reviewed commit: "fix(lockfile): surface unbalanced peer-s..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
finalize_lockfile_graph loaded pnpm-workspace.yaml with unwrap_or_default(), so a malformed workspace file silently fell back to an empty config and the stamped packageExtensionsChecksum / pnpmfileChecksum were computed from the wrong inputs, then persisted — desyncing config-drift detection. Propagate the parse error instead, matching the install entry path. A missing or empty workspace file is Ok(default), not an error, so single-package projects are unaffected. Co-authored-by: Cursor <cursoragent@cursor.com>
rubnogueira
marked this pull request as draft
June 14, 2026 23:49
Content-pinned git / remote-tarball deps were being split into multiple global-virtual-store entries, so a singleton package could load twice and throw "Cannot find module" at runtime. Three gaps, all closed here: - read: canonicalize a transitive git/tarball package's `name@<url>` head to the FS-safe `name@url+<hash>` form on parse (mirrors push_direct and a fresh resolve), so the parent's sibling symlink and the hasher's child lookup resolve instead of dangling. - resolve: add `resolve_dep_edge` and walk git/tarball `name@url+<hash>` edges in filter_graph / mark_optional_packages / mark_transitive_peer_dependencies so those subtrees aren't GC'd or mis-marked; keep globally-shareable keys bare (pnpm never suffixes a non-registry depPath) and don't fold meta-only optional peers into the dep_path. - prewarm: skip materializing content-affected nodes (a shareable source dep or any ancestor of one). Prewarm runs before fetch and hashes them content-less while the link phase hashes them content-ful; materializing them early stranded a duplicate content-less cohort with dangling source leaves. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/aube-resolver/src/platform.rs (1)
271-277:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
resolve_dep_edgewhen pruning transitive optionals.This pass still checks only
tail/name@tail, so a platform-mismatched optional git/tarball child keyed asname@url+<hash>is not removed. The later GC now resolves that same edge correctly and keeps the unsupported optional package reachable.Suggested fix
- let child_key = if package_keys.contains(tail) { - tail.clone() - } else { - format!("{name}@{tail}") - }; - let keep = !ignored.contains(name) && !mismatched_packages.contains(&child_key); + let child_is_mismatched = + match aube_lockfile::resolve_dep_edge(name, tail, |k| package_keys.contains(k)) { + Some(child_key) => mismatched_packages.contains(&child_key), + None => false, + }; + let keep = !ignored.contains(name) && !child_is_mismatched;🤖 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/aube-resolver/src/platform.rs` around lines 271 - 277, The retain closure filtering pkg.optional_dependencies is not correctly resolving dependency edges for git/tarball optional dependencies with hashes (keyed as name@url+<hash>). Replace the current inline logic that checks package_keys.contains(tail) and constructs the child_key with a call to resolve_dep_edge to properly resolve the dependency edge, which will handle all dependency types correctly and ensure platform-mismatched optional packages are properly removed.
🤖 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.
Outside diff comments:
In `@crates/aube-resolver/src/platform.rs`:
- Around line 271-277: The retain closure filtering pkg.optional_dependencies is
not correctly resolving dependency edges for git/tarball optional dependencies
with hashes (keyed as name@url+<hash>). Replace the current inline logic that
checks package_keys.contains(tail) and constructs the child_key with a call to
resolve_dep_edge to properly resolve the dependency edge, which will handle all
dependency types correctly and ensure platform-mismatched optional packages are
properly removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 5bf6b2fd-5fa1-4143-8ff6-2742af9d548d
📒 Files selected for processing (9)
crates/aube-lockfile/src/graph_hash.rscrates/aube-lockfile/src/lib.rscrates/aube-lockfile/src/pnpm/read.rscrates/aube-lockfile/src/pnpm/tests.rscrates/aube-lockfile/src/source.rscrates/aube-resolver/src/peer_context.rscrates/aube-resolver/src/platform.rscrates/aube-resolver/src/tests.rscrates/aube/src/commands/install/materialize.rs
✅ Files skipped from review due to trivial changes (1)
- crates/aube-lockfile/src/lib.rs
rubnogueira
marked this pull request as ready for review
June 16, 2026 00:52
…onals The optional-dependency prune still matched only the `tail` / `name@tail` conventions, so a platform-mismatched optional git/tarball child keyed as `name@url+<hash>` survived the prune and was only dropped (if at all) by the later GC. Route it through `resolve_dep_edge` like the other graph walks so the mismatched optional is removed here too. Co-authored-by: Cursor <cursoragent@cursor.com>
…ing them A pnpm dep_path peer suffix with an unclosed `(` (a truncated or hand-corrupted lockfile entry) made the paren walker return no segments, so the rewrite silently dropped everything after the `(` and shortened the key — a quiet lookup miss / mis-resolve on the next install. Preserve the bytes verbatim and emit WARN_AUBE_LOCKFILE_MALFORMED_PEER_SUFFIX so the bad entry is diagnosable. Also cross-reference the resolver's twin `outer_paren_segments` so the two copies don't diverge silently. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
|
@CodeRabbit resume |
✅ Action performedReviews resumed. |
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a cluster of
pnpm-lock.yamlparity and re-resolution bugs surfaced while runningaube install/aube upgradein asharedWorkspaceLockfile=falsemonorepo. After these changes aube's lockfile output and up-to-date checks match pnpm's, so a project that installs cleanly under pnpm no longer re-resolves on every install or writes a divergentlockfile under aube.
What's fixed
Each item is a separate commit.
Stop peer suffixes at the package that supplies the peer (
fix(resolver)) — a transitively-needed optional peer (e.g.fdir'spicomatch) leaked up the whole chain, so aube wrotexml2json@0.12.0(picomatch@4.0.4)where pnpm writesxml2json@0.12.0. The suffix is now suppressed once it reaches a package that provides the peer; the declarer keeps its suffix.Write hosted-git deps as codeload tarballs (
fix(resolver)) — a github/gitlab/bitbucket dep pinned to a 40-char SHA is now recorded as a codeloadRemoteTarball(name@https://codeload.github.com/<owner>/<repo>/tar.gz/<sha>withresolution: {gitHosted, integrity, tarball}+version:), matching pnpm, instead of the<url>.git#<sha>/type: gitform.resolution:keys are sorted to match pnpm's emitter.Render git/tarball peer suffixes as resolved specs (
fix(lockfile)) — completes ci(release-plz): grant contents:write to upload-assets caller #2 for peer suffixes:request-promise-core@1.1.4(request@https://codeload.github.com/<owner>/<repo>/tar.gz/<sha>)instead of aube's internal hashed(request@git+<hash>). Implemented as a symmetric translation (hashed→spec on write, spec→hashed on read) so a round-trip is a fixed point — a one-sided change would re-key every install and churn the lockfile.Stamp pnpm config checksums on update/remove/dedupe/audit (
fix(install)) — only the main install path wrotepackageExtensionsChecksum/pnpmfileChecksum; the other four commands resolved a fresh graph and dropped both fields. They now route through a sharedfinalize_lockfile_graphthat stamps the checksums, honoring--ignore-pnpmfile/--pnpmfile.Re-resolve when the pnpmfile changes (
fix(install)) — the warm-path settings hash never folded in the pnpmfile's own bytes, so editing a local.pnpmfile.{cjs,mjs}readPackage/afterAllResolvedhook rode the warm path: the hook silently never re-applied (noreadPackageoutput, stalenode_modules+ lockfile). The resolved pnpmfile's path + content is now hashed into the settings hash, mirroring pnpm's own up-to-date check. This also resolves the "lockfile doesn't regenerate whennode_modulesis out of sync" report.Design note
The peer-suffix translation (#3) is intentionally symmetric and restricted to git / remote-tarball sources, so the writer's hashed→spec pass and the reader's spec→hashed pass are exact inverses. A divergence would re-key the graph on
read and churn the lockfile on the next install (the original re-resolve loop).
Test plan
cargo test -p aube-lockfile(337 + new) and-p aube-resolvergreenrewrite_peer_suffixwalker (flat / nested / registry no-op / multi-segment / no-suffix)cargo clippy --all-targets -- -D warningsclean;cargo fmt --checkcleansharedWorkspaceLockfile=falsemonorepo: hosted-git deps + peer suffixes now match pnpm, a forced re-resolve round-trips with no churn, and.pnpmfilereadPackageoutput appears under aubeSummary by CodeRabbit
WARN_AUBE_LOCKFILE_MALFORMED_PEER_SUFFIXto surface malformed pnpmdep_pathpeer-suffix tails verbatim.dep_pathpeer-suffix parsing/rewriting (including nested peers) and ensured correct resolved-spec round-trips, preserving unbalanced parentheses verbatim.