You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
In practice it means that each package name will only map to a single version.
In the future the resolution could use the general greedy resolution.
The lockfile contains the resolution for all islands.
Islands can currently be used to have node_modules install within a PnP project, although I need to stress that they are highly experimental and will definitely change. This is reflected in the way they are configured (through a yarnrc setting called unstableIslands).
Not safe to merge as-is: contains an unsound unsafe block that is cancel-unsafe (use-after-free risk) and a VersionSet contract violation that can produce incorrect PubGrub solutions.
Two P1 issues exist: (1) the unsafe transmute is not cancel-safe — dropping the island resolution future mid-flight leaves a blocking thread with a dangling reference; (2) IslandVersionSet::full() violates the VersionSet contract, which is a correctness invariant that PubGrub depends on. The P2 issues (workspace hash missing island deps, hard-coded NPM registry) are non-blocking for a clearly experimental feature but should be addressed before wider adoption.
New file implementing island resolution via PubGrub; contains an unsound unsafe transmute of ctx to 'static that is not cancel-safe, and a completely disabled lockfile fast-path (dead code).
packages/zpm/src/island_types.rs
Defines IslandVersionSet VersionSet impl; full() returns a Semver range that incorrectly returns false for non-semver versions via contains, violating the VersionSet contract.
packages/zpm/src/island_provider.rs
New PubGrub DependencyProvider; hardcodes NPM registry in fetch_versions, missing support for custom/scoped registries.
packages/zpm/src/install.rs
Integrates island resolution into the main install pipeline; compute_workspace_hash only consults normalized_resolutions (greedy), missing island package dependencies from the hash.
packages/zpm/src/linker/nm/mod.rs
Adds link_island_nm for per-workspace node_modules linking for islands; logic looks correct for the single-workspace-per-island case.
packages/zpm/src/lockfile.rs
Extends Lockfile struct with an islands field and adds serialisation/deserialisation support; island entries share the main entries map for checksums, which looks intentional and correct.
packages/zpm-semver/src/pubgrub.rs
New module converting zpm semver Range tokens to pubgrub Ranges<PubgrubVersion>; conversions look correct for the handled operators.
packages/zpm/src/linker/mod.rs
Adds per-island link step after the main project link; currently only NodeModules linker is supported, consistent with the PR description.
Adds acceptance tests covering basic island scenarios (single/multiple islands, no-deps, empty islands error); good coverage of the happy path but no tests for cancellation safety, custom registries, or lockfile staleness with dependency changes.
Unsafe transmute is unsound on future cancellation
The SAFETY comment claims the transmuted reference is valid because the JoinHandle is .awaited immediately. This is incorrect when the resolve_island future is dropped/cancelled (e.g., by try_join in resolve_and_fetch when another future fails).
When a JoinHandle future is dropped in Tokio, the associated blocking thread is NOT aborted — it continues executing. After resolve_island is cancelled, ctx may be freed while the blocking thread still holds ctx_static, producing a use-after-free.
Repro scenario:
Island A and island B resolve concurrently via try_join_all.
Island A's pubgrub spawn_blocking task starts, holding ctx_static.
Island B's resolution returns an error, causing try_join to cancel island A's future.
Island A's JoinHandle future is dropped; the blocking thread keeps running against freed memory.
The safe alternative is to use a channel or Arc to pass ownership to the blocking task instead of a raw reference:
// Instead of transmuting ctx, clone the data the blocking task needs into owned values:let enforced_resolutions = ctx.enforced_resolutions.clone();let package_cache = ctx.package_cache.map(|c| c.clone_arc());// … pass owned values into spawn_blocking
Or drive the pubgrub solver incrementally in async context to avoid spawn_blocking entirely (as the existing TODO comment at line 184 already suggests).
packages/zpm/src/island_types.rs, line 286-289 (link)
full() violates the VersionSet contract for non-semver versions
full() returns Semver(Ranges::full()), but the contains implementation returns false for any version whose .version() is None (workspace packages, link:, portal:, etc.):
The VersionSet contract requires full().contains(v) == true for all versions v. Breaking this invariant can cause pubgrub to skip valid candidates or produce incorrect "no solution" errors for packages that only have non-semver versions.
The current design avoids hitting this because workspace and link: packages are always pre-constrained to exact singletons before pubgrub sees them. However, pubgrub internally uses full() when initialising package constraints (complement of empty) and the invariant should be maintained for correctness:
fnfull() -> Self{// Use NoneOf([]) — the exact "everything" set — so non-semver// versions are also covered without ambiguity.IslandVersionSet::Exact(ExactSet::NoneOf(SmallVec::new()))}
E2E test suggestion: add a test with a workspace package that has a link: dependency to another workspace and confirm the island resolves without error.
packages/zpm-semver/src/pubgrub.rs, line 100-106 (link)
LessThanOrEqual conversion is incorrect — it excludes the exact version
The <=v case first computes strictly_lower_than(v) and then unions it with singleton(v). This is mathematically correct. However, the initial range is built with a clone of version for strictly_lower_than, which is fine.
Wait — actually looking more carefully, this is correct: < v ∪ {v} = <= v. ✓
Disregard — there is no bug here. (Leaving this note so reviewers don't have to re-examine the same line.)
Prompt To Fix All With AI
This is a comment left during a code review.
Path: packages/zpm/src/island.rs
Line: 153-201
Comment:
**Unsafe transmute is unsound on future cancellation**
The SAFETY comment claims the transmuted reference is valid because the `JoinHandle` is `.await`ed immediately. This is incorrect when the `resolve_island` future is **dropped/cancelled** (e.g., by `try_join` in `resolve_and_fetch` when another future fails).
When a `JoinHandle` future is dropped in Tokio, the associated **blocking thread is NOT aborted** — it continues executing. After `resolve_island` is cancelled, `ctx` may be freed while the blocking thread still holds `ctx_static`, producing a use-after-free.
Repro scenario:
1. Island A and island B resolve concurrently via `try_join_all`.
2. Island A's pubgrub `spawn_blocking` task starts, holding `ctx_static`.
3. Island B's resolution returns an error, causing `try_join` to cancel island A's future.
4. Island A's `JoinHandle` future is dropped; the blocking thread keeps running against freed memory.
The safe alternative is to use a channel or `Arc` to pass ownership to the blocking task instead of a raw reference:
```rust// Instead of transmuting ctx, clone the data the blocking task needs into owned values:letenforced_resolutions=ctx.enforced_resolutions.clone();
letpackage_cache=ctx.package_cache.map(|c|c.clone_arc());
// … pass owned values into spawn_blocking```
Or drive the pubgrub solver incrementally in async context to avoid `spawn_blocking` entirely (as the existing TODO comment at line 184 already suggests).
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/zpm/src/island_types.rs
Line: 286-289
Comment:
**`full()` violates the `VersionSet` contract for non-semver versions**`full()` returns `Semver(Ranges::full())`, but the `contains` implementation returns `false` for any version whose `.version()` is `None` (workspace packages, `link:`, `portal:`, etc.):
```rustIslandVersionSet::Semver(r) => {
matchv.version() {
Some(version) =>r.contains(&PubgrubVersion::new(version)),
None=>false, // ← "full" range excludes non-semver versions!
}
}
```
The `VersionSet` contract requires `full().contains(v) == true` for **all** versions `v`. Breaking this invariant can cause pubgrub to skip valid candidates or produce incorrect "no solution" errors for packages that only have non-semver versions.
The current design avoids hitting this because workspace and `link:` packages are always pre-constrained to exact singletons before pubgrub sees them. However, pubgrub internally uses `full()` when initialising package constraints (complement of empty) and the invariant should be maintained for correctness:
```rustfnfull() ->Self {
// Use NoneOf([]) — the exact "everything" set — so non-semver// versions are also covered without ambiguity.IslandVersionSet::Exact(ExactSet::NoneOf(SmallVec::new()))
}
```
E2E test suggestion: add a test with a workspace package that has a `link:` dependency to another workspace and confirm the island resolves without error.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/zpm/src/install.rs
Line: 1082-1119
Comment:
**`compute_workspace_hash` misses island package dependencies**`compute_workspace_hash` traverses `self.result.install_state.normalized_resolutions`, but island resolutions live in `self.result.install_state.island_normalized_resolutions`. Workspace locators that belong to an island will not find their dependencies here and will be hashed as bare locators.
Consequence: adding, upgrading, or removing a dependency in an island workspace won't change the workspace hash stored in `lockfile.workspaces`. Lockfile-staleness checks that rely on this hash (e.g. immutable installs) will incorrectly report no change.
An E2E test demonstrating this: run `install` with an island workspace that has a dependency, then update the dependency version in its `package.json` and run `install --immutable` — it should fail, but currently it may pass silently.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/zpm/src/island_provider.rs
Line: 105-113
Comment:
**`fetch_versions` always uses the NPM registry, ignoring custom registries**`Registry::Npm(package.clone())` hard-codes the npm registry for every package. Packages from scoped registries (e.g. `@company/foo` resolving from a private registry configured via `npmRegistryServer` or `npmScopes`) will be fetched from the wrong endpoint.
Consider deriving the registry from the package's scope using the project configuration (similar to how `http_npm::get_registry` is called elsewhere), or at minimum documenting this limitation prominently.
E2E test suggestion: add an island test with a scoped package (`@scope/pkg`) that points to a custom registry and assert it resolves correctly.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/zpm-semver/src/pubgrub.rs
Line: 100-106
Comment:
**`LessThanOrEqual` conversion is incorrect — it excludes the exact version**
The `<=v` case first computes `strictly_lower_than(v)` and then unions it with `singleton(v)`. This is mathematically correct. However, the initial range is built with a **clone** of `version` for `strictly_lower_than`, which is fine.
Wait — actually looking more carefully, this is correct: `< v ∪ {v} = <= v`. ✓
Disregard — there is no bug here. (Leaving this note so reviewers don't have to re-examine the same line.)
How can I resolve this? If you propose a fix, please make it concise.
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
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.
This PR implements a very experimental concept of islands.
node-moduleslinker is supported at this time.Islands can currently be used to have
node_modulesinstall within a PnP project, although I need to stress that they are highly experimental and will definitely change. This is reflected in the way they are configured (through a yarnrc setting calledunstableIslands).