Repository navigation
fix(install): map peer-suffixed source deps to their canonical store index - #885
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR standardizes parenthesized peer-context suffix handling, updates lockfile canonicalization to strip suffixes at the first ChangesPeer-suffix canonicalization and remapping
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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. Comment |
Greptile SummaryThis PR fixes
Confidence Score: 4/5The fetch-layer fix is precise and well-tested; the peer-context format change is internally consistent. The unresolved concern (already raised in a prior review) is that removing _<10hex> handling from the lockfile layout functions breaks parsing of any lockfile written by a previous aube version when the suffix cap was hit. The core fetch.rs bug fix and peer_context.rs format migration are both correct. However, layout.rs no longer handles the old underscore-hex peer marker, so any project that previously triggered the suffix cap will have its existing lockfile misread after upgrading — canonical_key_from_dep_path, child_canonical_key, and dep_value_as_version silently return keys with the _ trailer still attached, which won't match any entry in the canonical map. crates/aube-lockfile/src/npm/layout.rs — the three key-derivation functions no longer handle the old _<10hex> suffix format Important Files Changed
Reviews (3): Last reviewed commit: "ci: fix lint" | Re-trigger Greptile |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/aube/src/commands/install/fetch.rs (1)
1105-1126: 💤 Low valueConsider consolidating duplicate
strip_hashed_peer_suffiximplementations.This function is identical to
strip_hashed_peer_suffixincrates/aube-lockfile/src/npm/layout.rs(line 238). If the peer-suffix format changes in the future, both implementations would need to stay in sync.Consider exposing a shared utility (e.g.,
pub(crate)inaube-lockfilewith a re-export, or a small shared crate) so the stripping logic lives in one place. Not blocking since both are currently correct and well-tested.🤖 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/src/commands/install/fetch.rs` around lines 1105 - 1126, Duplicate function strip_hashed_peer_suffix should be consolidated into a single shared utility and referenced from both places: move the implementation into a common location (for example, add a pub(crate) function strip_hashed_peer_suffix in the aube-lockfile crate or a small shared crate), export/re-export it so callers can use the same symbol, then replace the duplicate local implementations by calling that shared strip_hashed_peer_suffix function; ensure visibility and imports are updated and run tests to confirm behavior is unchanged.
🤖 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/aube/src/commands/install/fetch.rs`:
- Around line 1105-1126: Duplicate function strip_hashed_peer_suffix should be
consolidated into a single shared utility and referenced from both places: move
the implementation into a common location (for example, add a pub(crate)
function strip_hashed_peer_suffix in the aube-lockfile crate or a small shared
crate), export/re-export it so callers can use the same symbol, then replace the
duplicate local implementations by calling that shared strip_hashed_peer_suffix
function; ensure visibility and imports are updated and run tests to confirm
behavior is unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 241385b2-2d0d-482a-b451-8479ccaf6ef2
📒 Files selected for processing (1)
crates/aube/src/commands/install/fetch.rs
|
I cross-checked this against current pnpm source ( pnpm keeps the package's base identity separately from the peer-context dep path: So mapping source-backed contextualized dep paths like Important compatibility issue I noticed while checking this: current pnpm's long peer suffix hash is still parenthesized, i.e. This comment was generated by Codex. |
The peer-context pass capped over-long peer suffixes with a bare `_<10hex>` marker, but pnpm's createPeerDepGraphHash always returns a parenthesized `(<shortHash>)` — sha256 hex truncated to 32 chars, hashing the suffix body (peers joined with `)(`, without the outer parens). Writing the bare form into pnpm-lock.yaml is a lockfile compatibility bug. Emit pnpm's parenthesized form. Because the capped suffix is now a normal parenthesized segment, the bare-marker strippers in install/fetch and aube-lockfile are redundant and removed — the existing first-`(` split recovers the canonical key — which also dissolves the duplicate-helper nitpick. Co-authored-by: Cursor <cursoragent@cursor.com>
…fix/invalid-canonical-dep
Summary
Fresh
--forceinstalls that pull a git or remote-tarball dependency which acquires a peer-context suffix (directly or via propagation from a peer-bearing descendant) could fail late in linking withERR_AUBE_MISSING_PACKAGE_INDEX— e.g.some-pkg@git+<sha>(react@18.2.0)"is in the link graph but missing from package_indices".Root cause: the streaming fetch keys source-dep indices under their canonical source coordinate (
name@git+<short>), but the resolver's peer-context post-pass rewrites the graph dep_path toname@git+<short>(peer@ver).remap_indices_to_contextualizedonly matched the full contextualized dep_path orspec_key()(semver), so the source-coordinate key was never found and the index was dropped.Fix: strip the peer-context suffix (parenthesized
(peer@ver)tail or hashed_<10hex>marker) to recover the canonical coordinate and look the index up under it. This is the precise inverse of how the peer-context pass builds{canonical_base}{suffix}, so it reconstructs the exact streamed key. The post-filter_graphcatch-up remains as a fallback, so coverage strictly improves.Test plan
cargo test -p aube --lib remap/strip_peer_context— 4 newunit tests (parenthesized suffix, hashed suffix, suffixless, and
the suffix-stripper helper)
cargo clippy -p aube— cleangit+file://dep that is an ancestor of aregistry package peering on
react, installed alongsidereact,installs cleanly into the global virtual store
aube install --forceon the affectedproject with the rebuilt binary
Summary by CodeRabbit
Bug Fixes
(peer@ver)and(<short-hash>)peer suffix forms.Documentation
(<short-hash>).Tests