fix(install): workspace self dependencies with isolated linker #23609
Conversation
|
Updated 4:04 PM PT - Oct 13th, 2025
❌ @Jarred-Sumner, your commit 10bcd32 has some failures in 🧪 To try this PR locally: bunx bun-pr 23609That installs a local version of the PR into your bun-23609 --bun |
WalkthroughAdjusts cycle-detection in isolated install to consider workspace behavior alongside name hash, ensuring self-referential workspace dependencies are not skipped. Adds a test validating symlink creation for self-referencing and cross-referencing workspace dependencies in isolated installs. Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks✅ Passed checks (4 passed)
Comment |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/install/isolated_install.zig(1 hunks)test/cli/install/isolated-install.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/install/isolated_install.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/install/isolated_install.zig
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/cli/install/isolated-install.test.ts
test/cli/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/cli/**/*.{js,ts}: Place CLI command tests (e.g., bun install, bun init) under test/cli/
When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Files:
test/cli/install/isolated-install.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/cli/install/isolated-install.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or roll your own random port
Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Prefer running tests via bun bd test and use provided harness utilities (bunEnv, bunExe, tempDir)
Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Files:
test/cli/install/isolated-install.test.ts
test/cli/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place CLI command tests under test/cli/
Files:
test/cli/install/isolated-install.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testfor files ending with*.test.{ts,js,jsx,tsx,mjs,cjs}
Prefer concurrent tests (test.concurrent/describe.concurrent) over sequential when feasible
Organize tests withdescribeblocks to group related tests
Use utilities likedescribe.each,toMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach) and track resources for cleanup
Files:
test/cli/install/isolated-install.test.ts
test/**/*.{ts,tsx,js,jsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
For large/repetitive strings, use
Buffer.alloc(count, fill).toString()instead of"A".repeat(count)
Files:
test/cli/install/isolated-install.test.ts
🧬 Code graph analysis (1)
test/cli/install/isolated-install.test.ts (1)
test/harness.ts (3)
runBunInstall(1215-1271)bunEnv(49-64)readdirSorted(1768-1772)
🔇 Additional comments (1)
src/install/isolated_install.zig (1)
88-97: Cycle-skip guard refined correctly; confirmbehavior.workspacesemanticsCondition now checks same name and same workspace behavior before skipping. That prevents skipping implicit vs explicit workspace deps. LGTM.
Please confirm
behavior.workspaceis the intended flag (and available here). In most places this file usesbehavior.isWorkspace()orversion.tag == .workspace. Ifbehavior.workspaceisn’t exposed, consider:
- Comparing explicitness via tag:
const same_workspace_kind = (curr_dep.version.tag == .workspace) == (entry_dep.version.tag == .workspace);
and use it in the condition.
| test("workspace self dependencies create symlinks", async () => { | ||
| const { packageDir } = await registry.createTestDir({ | ||
| bunfigOpts: { isolated: true }, | ||
| files: { | ||
| "package.json": JSON.stringify({ | ||
| name: "monorepo-workspace-self-dep", | ||
| workspaces: ["packages/*"], | ||
| }), | ||
| "packages/pkg1/package.json": JSON.stringify({ | ||
| name: "pkg1", | ||
| dependencies: { | ||
| pkg1: "workspace:*", | ||
| }, | ||
| }), | ||
| "packages/pkg2/package.json": JSON.stringify({ | ||
| name: "pkg2", | ||
| dependencies: { | ||
| "pkg1": "workspace:*", | ||
| "pkg2": "workspace:*", | ||
| }, | ||
| }), | ||
| "packages/pkg3/package.json": JSON.stringify({ | ||
| name: "pkg3", | ||
| dependencies: { | ||
| "different-name": "workspace:.", | ||
| }, | ||
| }), | ||
| }, | ||
| }); | ||
|
|
||
| await runBunInstall(bunEnv, packageDir); | ||
|
|
||
| expect( | ||
| await Promise.all([ | ||
| readdirSorted(join(packageDir, "node_modules")), | ||
| file(join(packageDir, "packages", "pkg1", "node_modules", "pkg1", "package.json")).json(), | ||
| file(join(packageDir, "packages", "pkg2", "node_modules", "pkg1", "package.json")).json(), | ||
| file(join(packageDir, "packages", "pkg2", "node_modules", "pkg2", "package.json")).json(), | ||
| file(join(packageDir, "packages", "pkg3", "node_modules", "different-name", "package.json")).json(), | ||
| ]), | ||
| ).toEqual([ | ||
| [".bun"], | ||
| { name: "pkg1", dependencies: { pkg1: "workspace:*" } }, | ||
| { name: "pkg1", dependencies: { pkg1: "workspace:*" } }, | ||
| { name: "pkg2", dependencies: { pkg1: "workspace:*", pkg2: "workspace:*" } }, | ||
| { name: "pkg3", dependencies: { "different-name": "workspace:." } }, | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Strengthen assertions: also verify these are symlinks and targets
Great coverage. Add explicit symlink checks to ensure links (not copies) and correct targets for self and alias cases.
@@
await runBunInstall(bunEnv, packageDir);
+ // verify symlink targets (not copies)
+ expect(lstatSync(join(packageDir, "packages", "pkg1", "node_modules", "pkg1")).isSymbolicLink()).toBeTrue();
+ expect(readlinkSync(join(packageDir, "packages", "pkg1", "node_modules", "pkg1"))).toBe(join("..", ".."));
+ expect(lstatSync(join(packageDir, "packages", "pkg2", "node_modules", "pkg1")).isSymbolicLink()).toBeTrue();
+ expect(readlinkSync(join(packageDir, "packages", "pkg2", "node_modules", "pkg1"))).toBe(join("..", "..", "pkg1"));
+ expect(lstatSync(join(packageDir, "packages", "pkg2", "node_modules", "pkg2")).isSymbolicLink()).toBeTrue();
+ expect(readlinkSync(join(packageDir, "packages", "pkg2", "node_modules", "pkg2"))).toBe(join("..", ".."));
+ expect(lstatSync(join(packageDir, "packages", "pkg3", "node_modules", "different-name")).isSymbolicLink()).toBeTrue();
+ expect(readlinkSync(join(packageDir, "packages", "pkg3", "node_modules", "different-name"))).toBe(join("..", ".."));
+
expect(
await Promise.all([
readdirSorted(join(packageDir, "node_modules")),
file(join(packageDir, "packages", "pkg1", "node_modules", "pkg1", "package.json")).json(),
file(join(packageDir, "packages", "pkg2", "node_modules", "pkg1", "package.json")).json(),Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In test/cli/install/isolated-install.test.ts around lines 420 to 467, the
assertions check package.json contents but don't assert that the workspace
installs are symlinks or that they point to the correct workspace targets;
update the test to lstat the relevant node_modules paths
(packageDir/node_modules/.bun? no — specifically
packageDir/packages/pkg1/node_modules/pkg1,
packageDir/packages/pkg2/node_modules/pkg1,
packageDir/packages/pkg2/node_modules/pkg2,
packageDir/packages/pkg3/node_modules/different-name) and assert each is a
symbolic link, then readlink each and assert the resolved target matches the
corresponding workspace package directory (e.g. packages/pkg1 for pkg1,
packages/pkg2 for pkg2, and packages/pkg3 for different-name); keep the existing
JSON content assertions but add these symlink and target checks immediately
before or after them.
3 participants
What does this PR do?
Fixes a bug preventing workspace self dependencies from getting symlinked to the workspace node_modules
Fixes #23605
How did you verify your code works?
Added a test for normal
"workspace:*"deps, and"workspace:."under a different name.