Repository navigation
fix(linker): resolve git deps in global virtual store - #857
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughClassify Git/RemoteTarball as globally shareable, derive canonical dep_path keys for them, add index content fingerprinting, thread content hashes into graph-hash computation, materialize shared-store content with symlink wiring, and route linking/hoisting to use the shared global virtual store when enabled. ChangesGlobal Virtual Store for Git and Remote-Tarball Dependencies
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. Comment |
Greptile SummaryThis PR fixes two independent defects in the isolated linker's global-virtual-store (GVS) path that caused dangling sibling symlinks when a registry package depended on a git or remote-tarball dep: (1) shareable local sources were never materialized into the shared store, and (2) dependents looked up the sibling by its raw resolved spec rather than the hashed
Confidence Score: 5/5Safe to merge — the fix is scoped entirely to the global-virtual-store code path; non-GVS installs are structurally unchanged. The implementation correctly mirrors pnpm::read::push_direct keying at every call site, backward compatibility is preserved via a no-op content hash delegation, and the test suite covers canonical spec forms, content-fingerprint cascade, engine-taint cascade, and the fail-loud contract for missing indices. The only observation is a diagnostic quality nit in stale-entry cleanup with no correctness impact. No files require special attention; materialize.rs has one minor diagnostic quality note but no correctness issue. Important Files Changed
Reviews (6): Last reviewed commit: "fix: apply ai review suggestions" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 1 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
jdx
left a comment
jdx
left a comment
There was a problem hiding this comment.
Thanks for digging into this. I think the core linker fix is heading in the right direction: materializing git/remote-tarball deps into the shared GVS and canonicalizing sibling dep paths in materialize.rs / hoisted.rs matches the failure mode described here.
One thing I think still needs fixing before merge: crates/aube-lockfile/src/graph_hash.rs still resolves child deps with raw format!("{alias}@{tail}") lookups in both calc_deps_hash and transitively_requires_build.
For real pnpm lockfile shapes, a git/tarball dependency value can be the resolved URL, while graph.packages is keyed as name@git+<hash> / name@url+<hash>. That means the graph hasher can skip that child. Two consequences:
- the parent package’s GVS hash does not include the child’s content fingerprint
- build/engine taint from an allowlisted git/tarball child does not cascade to the parent
I verified this locally by adding URL-shaped regression tests: current PR code keeps the parent hash identical for both content-fingerprint changes and engine changes. Applying the same canonical child dep-path logic in graph_hash.rs makes both tests pass.
Could we move the canonicalization helper into aube-lockfile (or add an equivalent there) and use it from calc_deps_hash, transitively_requires_build, and the linker? I’d also add regression coverage where the parent dependency value is URL-shaped, e.g. https://github.com/request/request.git#<sha> or a codeload tarball URL, not the already-canonical synthetic git+abc.
Minor: the PR title is pretty vague. Something like fix(linker): resolve git deps in global virtual store would fit the repo conventions better.
This comment was generated by Codex.
rubnogueira
changed the title
![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 (2)
crates/aube-lockfile/src/lib.rs (1)
600-603:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCanonicalize URL-shaped children in
transitive_closuretoo.
filter_deps()andsubset_to_importer()still rebuild every edge asname@tail. For git / remote-tarball entries,tailis the resolved URL whilepackagesstores the node undershared_local_dep_path(...), so these BFS walks stop early and emit pruned graphs that are missing the whole transitive subtree.Suggested fix
- let child_key = format!("{child_name}@{child_version}"); + let child_key = shared_local_dep_path(child_name, child_version) + .unwrap_or_else(|| format!("{child_name}@{child_version}")); if reachable.insert(child_key.clone()) { queue.push_back(child_key); }🤖 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/lib.rs` around lines 600 - 603, The BFS in transitive_closure builds child keys as format!("{child_name}@{child_version}") which fails for git/remote-tarball nodes because packages stores those nodes under shared_local_dep_path(...) rather than the raw resolved URL; change the child_key construction in transitive_closure to canonicalize URL-shaped tails: detect when child_version is a URL/remote-tarball/git-form (using the same predicate used elsewhere) and instead compute the stored node key with shared_local_dep_path(child_name, &child_version) (or the existing canonicalizer used by filter_deps/subset_to_importer), otherwise keep the existing name@tail form, then use that canonical child_key for reachable.insert and queue.push_back. Also make the same canonicalization change in filter_deps and subset_to_importer wherever edges are rebuilt so all BFS/walk code uses the same stored-key format and the transitive subtree is not pruned.crates/aube-linker/src/link.rs (1)
165-178:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
aube_dir_entry_name(dep_path)for the single-project local cache probe.
materialize_into(..., false)writes local packages under.aube/<aube_dir_entry_name(dep_path)>, but this fast path still checks.aube/<dep_path>raw. For any dep_path that needs escaping or flattening, warm reruns will miss the existing entry and rematerialize into an already-populated destination. The workspace path below already uses the transformed name, solink_allshould match it.Proposed fix
- let aube_entry = aube_dir.join(dep_path); + let aube_entry = aube_dir.join(self.aube_dir_entry_name(dep_path));🤖 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-linker/src/link.rs` around lines 165 - 178, The existence check uses the raw dep_path but materialize_into(..., false) writes into the transformed name, causing false negatives; change the aube_entry construction in link_all to use aube_dir_entry_name(dep_path) (i.e. replace aube_dir.join(dep_path) with aube_dir.join(aube_dir_entry_name(dep_path))) so the fast path probes the same flattened/escaped path materialize_into uses and correctly increments stats.packages_cached.
🤖 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-lockfile/src/source.rs`:
- Around line 318-320: The current truncation using dep_value.split('(').next()
removes any '(' anywhere; instead only strip pnpm's trailing peer suffix: check
if dep_value ends_with(')') and rfind('(') to get the last '('; if the substring
after that '(' starts_with("peer@") then set classify to the slice before that
'(' otherwise leave classify as dep_value. Update the code that sets classify
(the line using dep_value.split('(').next().unwrap_or(dep_value)) to use this
conditional rfind/starts_with logic so only a terminal "(peer@...)" is removed.
---
Outside diff comments:
In `@crates/aube-linker/src/link.rs`:
- Around line 165-178: The existence check uses the raw dep_path but
materialize_into(..., false) writes into the transformed name, causing false
negatives; change the aube_entry construction in link_all to use
aube_dir_entry_name(dep_path) (i.e. replace aube_dir.join(dep_path) with
aube_dir.join(aube_dir_entry_name(dep_path))) so the fast path probes the same
flattened/escaped path materialize_into uses and correctly increments
stats.packages_cached.
In `@crates/aube-lockfile/src/lib.rs`:
- Around line 600-603: The BFS in transitive_closure builds child keys as
format!("{child_name}@{child_version}") which fails for git/remote-tarball nodes
because packages stores those nodes under shared_local_dep_path(...) rather than
the raw resolved URL; change the child_key construction in transitive_closure to
canonicalize URL-shaped tails: detect when child_version is a
URL/remote-tarball/git-form (using the same predicate used elsewhere) and
instead compute the stored node key with shared_local_dep_path(child_name,
&child_version) (or the existing canonicalizer used by
filter_deps/subset_to_importer), otherwise keep the existing name@tail form,
then use that canonical child_key for reachable.insert and queue.push_back. Also
make the same canonicalization change in filter_deps and subset_to_importer
wherever edges are rebuilt so all BFS/walk code uses the same stored-key format
and the transitive subtree is not pruned.
🪄 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: 07e09b60-903b-45c7-9752-3b86368b2b46
📒 Files selected for processing (6)
crates/aube-linker/src/hoisted.rscrates/aube-linker/src/link.rscrates/aube-linker/src/materialize.rscrates/aube-lockfile/src/graph_hash.rscrates/aube-lockfile/src/lib.rscrates/aube-lockfile/src/source.rs
| // pnpm appends a `(peer@ver)` suffix to some spec values; the parser | ||
| // strips it before classifying the source, so strip it here too. | ||
| let classify = dep_value.split('(').next().unwrap_or(dep_value); |
There was a problem hiding this comment.
Strip only a trailing peer suffix.
split('(').next() truncates any ( that belongs to the real spec, not just pnpm's terminal (peer@ver) suffix. A tarball URL like .../pkg(1).tgz or a git selector like ...&path:/packages/foo(bar) will canonicalize to a different dep_path than the parser inserted, so the linker/hasher can miss that child again.
🤖 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/source.rs` around lines 318 - 320, The current
truncation using dep_value.split('(').next() removes any '(' anywhere; instead
only strip pnpm's trailing peer suffix: check if dep_value ends_with(')') and
rfind('(') to get the last '('; if the substring after that '('
starts_with("peer@") then set classify to the slice before that '(' otherwise
leave classify as dep_value. Update the code that sets classify (the line using
dep_value.split('(').next().unwrap_or(dep_value)) to use this conditional
rfind/starts_with logic so only a terminal "(peer@...)" is removed.
…ith canonical sshurl resolved Upstream aubepkg#857 reclassified github/gitlab/bitbucket-shorthand git deps to resolve via a codeload archive — LocalSource::RemoteTarball { git_hosted: true } rather than LocalSource::Git. The npm writer's supplementary canonical-map patch filtered Git-only, so the git-hosted dep was dropped from package-lock.json entirely; real `npm ci` then rejected the lock with `Missing: <dep> from lock file`. Two coordinated edits, both default-preserving (a plain non-git RemoteTarball is unaffected; standalone aube's non-codeload paths are byte-for-byte unchanged): - write.rs: widen the canonical-map filter (now via is_git_local_source) to also include RemoteTarball { git_hosted: true } so the dep is placed. - source.rs: npm_resolved_field emits npm's canonical git+ssh://git@<host>/<owner>/<repo>.git#<sha> for a git-hosted codeload dep (inverting the codeload archive URL per provider) rather than the codeload tarball URL, matching real npm 11.13.0 so `npm install` doesn't churn the line.
fix(global-virtual-store): materialize git/remote-tarball deps in the shared store and key dependent siblings correctly
Summary
With
enableGlobalVirtualStore: true, a registry package that depends on a git or remote-tarball dependency gets a dangling sibling symlink to that dependency. At runtime the dependency resolves to nothing, so any code reaching into it throws.pnpmlinks the same graph correctly.The canonical trigger is
request-promise→request-promise-core, which doesrequire('request').Request.prototypewhenrequestis sourced from git:(Depending on how the dep was recorded, the same defect can also surface as
Error: Cannot find module 'request'.)Root cause
Two independent defects in the isolated linker's global-virtual-store (GVS) path:
Shareable local sources were never materialized into the shared store.
git/ remote-tarball packages are content-pinned and shared across projects exactly like registry packages, but they were only ever materialized into the per-project.aube/. A registry dependent living in the shared store points its dependency siblings at the hashed global path, so that sibling symlink dangled.Dependents keyed the sibling under the wrong identity. pnpm records a git/tarball dep inside a snapshot's
dependencies:map by its resolved spec (<url>#<sha>/ tarball URL), but keys the package itself under the shortname@git+<hash>/name@url+<hash>form. The linker's naiveformat!("{name}@{value}")sibling lookup therefore pointed at a virtual-store path that was never materialized.Changes
aube-lockfile/src/source.rs— addLocalSource::is_globally_shareable()(Git|RemoteTarball), with rationale on why these are shared-store-eligible whilefile:/link:/portal:/exec:stay per-project.aube-linker/src/materialize.rsshared_local_dep_path()— canonicalizes a recorded git/tarball spec to the samedep_paththe parser keys the package under (promoting#committish→resolvedfor git so the hash matches; strips pnpm's(peer@ver)suffix).ensure_shared_local_in_global_store()— materializes a shareable local source into the shared store and symlinks.aube/<dep_path>at it, clearing stale per-project dirs/links left by older aube versions.materialize_intosibling loop to useshared_local_dep_path.aube-linker/src/link.rs— bothlink_allandlink_workspacenow route shareable local sources throughensure_shared_local_in_global_storewhen GVS is enabled.aube-linker/src/hoisted.rs— apply the samedep_pathcanonicalization so the hoisted layout doesn't silently drop the dep's subtree.Behavior (before → after)
For
request-promise-core'srequestsibling under GVS:request -> ../../request@https+++codeload…/node_modules/request→ dangling.request -> ../../request@url+<hash>/node_modules/request(orgit+<hash>) → resolves.Reproduction
A minimal project that forces
request-promiseonto a git-sourcedrequest:pnpm-workspace.yamlpackage.jsondeps:{ "request": "catalog:", "request-promise": "4.2.6" }, then:aube install node -e "require('request-promise'); console.log('ok')"request).ok.Testing
shared_local_dep_path(github shorthand →git+, shorthand/URL convergence,committish→resolvedpromotion, codeload tarball →url+, peer-suffix stripping,Nonefor non-shareable specs).request-promiseloads.aube-linker+ 285aube-lockfiletests, no regressions.Compatibility / risk
ensure_shared_local_in_global_storeclears stale per-project entries from older aube versions on the next install.Repro: https://github.com/rubnogueira/aube-repro-gvs-exotic-hash
Summary by CodeRabbit
Bug Fixes
New Features
Tests