Repository navigation
Conversation
WalkthroughEnable following symlinks when scanning workspace glob patterns by flipping a GlobWalker flag in WorkspaceMap.zig, and add regression tests that validate discovery of symlinked workspace packages during bun install. Changes
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
🤖 Fix all issues with AI agents
In @test/regression/issue/25801.test.ts:
- Around line 68-86: Remove the dead initial creation block (the first mkdirSync
and the first symlinkSync that point package.json into packages/pkg-alias) since
it is immediately removed; keep only the external tempDir creation and the final
symlinkSync that links externalPkgDir into packages/pkg-alias. Replace the shell
removal call Bun.$`rm -rf ${...}` with a native filesystem removal like
fs.rmSync(join(String(monorepoDir), "packages", "pkg-alias"), { recursive: true,
force: true }) or await fs.promises.rm(..., { recursive: true, force: true }) so
no shell command is used.
📜 Review details
Configuration used: Path: .coderabbit.yaml
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.
📥 Commits
Reviewing files that changed from the base of the PR and between beccd01 and b5924f91b9866cc8756f8fd5f982807794235ab1.
📒 Files selected for processing (2)
src/install/lockfile/Package/WorkspaceMap.zigtest/regression/issue/25801.test.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, be careful with allocators and use defer for cleanup
Files:
src/install/lockfile/Package/WorkspaceMap.zig
src/**/*.zig
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.zig: Use the#prefix for private fields in Zig structs, e.g.,struct { #foo: u32 };
Use Decl literals in Zig, e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };
Place@importstatements at the bottom of the file in Zig (auto formatter will handle positioning)
Never use@import()inline inside functions in Zig; always place imports at the bottom of the file or containing struct
Files:
src/install/lockfile/Package/WorkspaceMap.zig
**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts?(x): Never usebun testdirectly - always usebun bd testto run tests with debug build changes
For single-file tests, prefer-eflag overtempDir
For multi-file tests, prefertempDirandBun.spawnover single-file tests
UsenormalizeBunSnapshotto normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
UsetempDirfromharnessto create temporary directories - do not usetmpdirSyncorfs.mkdtempSync
When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Do not write flaky tests - do not usesetTimeoutin tests; instead await the condition to be met
Verify tests fail withUSE_SYSTEM_BUN=1 bun test <file>and pass withbun bd test <file>- tests are invalid if they pass with USE_SYSTEM_BUN=1
Test files must end with.test.tsor.test.tsx
Avoid shell commands likefindorgrepin tests - use Bun's Glob and built-in tools instead
Files:
test/regression/issue/25801.test.ts
test/regression/issue/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Place regression tests for specific GitHub issues in
test/regression/issue/${issueNumber}.test.tswith real issue numbers only
Files:
test/regression/issue/25801.test.ts
test/**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
port: 0in tests - do not hardcode ports or use custom random port number functions
Files:
test/regression/issue/25801.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 bd test <...test file>to run tests with compiled code changes. Do not usebun testas it will not include your changes.
Usebun:testfor files ending in*.test.{ts,js,jsx,tsx,mjs,cjs}. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, usebun bd <file>instead ofbun bd test <file>since they expect exit code 0.
Do not set a timeout on tests. Bun already has timeouts built-in.
Files:
test/regression/issue/25801.test.ts
🧠 Learnings (14)
📚 Learning: 2026-01-05T16:32:07.551Z
Learnt from: alii
Repo: oven-sh/bun PR: 25474
File: src/bun.js/event_loop/Sigusr1Handler.zig:0-0
Timestamp: 2026-01-05T16:32:07.551Z
Learning: In Zig codebases (e.g., Bun), treat std.posix.sigaction as returning void and do not perform runtime error handling for its failure. The Zig standard library views sigaction failures as programmer errors (unreachable) because they only occur with invalid signals like SIGKILL/SIGSTOP. Apply this pattern across Zig files that call sigaction (e.g., crash_handler.zig, main.zig, filter_run.zig, process.zig) and ensure failures are not handled as recoverable errors; prefer reaching an explicit unreachable/compile-time assumption when such failures are detected.
Applied to files:
src/install/lockfile/Package/WorkspaceMap.zig
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
test/regression/issue/25801.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Applied to files:
test/regression/issue/25801.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun bd test <...test file>` to run tests with compiled code changes. Do not use `bun test` as it will not include your changes.
Applied to files:
test/regression/issue/25801.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : For multi-file tests, prefer `tempDir` and `Bun.spawn` over single-file tests
Applied to files:
test/regression/issue/25801.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to test/regression/issue/*.test.ts : Place regression tests for specific GitHub issues in `test/regression/issue/${issueNumber}.test.ts` with real issue numbers only
Applied to files:
test/regression/issue/25801.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending in `*.test.{ts,js,jsx,tsx,mjs,cjs}`. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, use `bun bd <file>` instead of `bun bd test <file>` since they expect exit code 0.
Applied to files:
test/regression/issue/25801.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Verify tests fail with `USE_SYSTEM_BUN=1 bun test <file>` and pass with `bun bd test <file>` - tests are invalid if they pass with USE_SYSTEM_BUN=1
Applied to files:
test/regression/issue/25801.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Use `normalizeBunSnapshot` to normalize snapshot output of tests
Applied to files:
test/regression/issue/25801.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Avoid shell commands like `find` or `grep` in tests - use Bun's Glob and built-in tools instead
Applied to files:
test/regression/issue/25801.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Never use `bun test` directly - always use `bun bd test` to run tests with debug build changes
Applied to files:
test/regression/issue/25801.test.ts
📚 Learning: 2025-10-26T01:32:04.844Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24082
File: test/cli/test/coverage.test.ts:60-112
Timestamp: 2025-10-26T01:32:04.844Z
Learning: In the Bun repository test files (test/cli/test/*.test.ts), when spawning Bun CLI commands with Bun.spawnSync for testing, prefer using stdio: ["inherit", "inherit", "inherit"] to inherit stdio streams rather than piping them.
Applied to files:
test/regression/issue/25801.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Organize unit tests by module in directories like `/test/js/bun/` and `/test/js/node/`.
Applied to files:
test/regression/issue/25801.test.ts
📚 Learning: 2025-11-06T00:58:23.965Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24417
File: test/js/bun/spawn/spawn.test.ts:903-918
Timestamp: 2025-11-06T00:58:23.965Z
Learning: In Bun test files, `await using` with spawn() is appropriate for long-running processes that need guaranteed cleanup on scope exit or when explicitly testing disposal behavior. For short-lived processes that exit naturally (e.g., console.log scripts), the pattern `const proc = spawn(...); await proc.exited;` is standard and more common, as evidenced by 24 instances vs 4 `await using` instances in test/js/bun/spawn/spawn.test.ts.
Applied to files:
test/regression/issue/25801.test.ts
🧬 Code graph analysis (1)
test/regression/issue/25801.test.ts (1)
test/harness.ts (1)
tempDir(277-284)
🔇 Additional comments (2)
test/regression/issue/25801.test.ts (1)
10-50: Test structure looks good.The test correctly validates the fix by creating an external package, symlinking it into the monorepo, and verifying
bun installsucceeds without "Workspace dependency not found" errors.Minor: The
stdoutvariable on line 42 is captured but never used. Consider removing it or adding an assertion if there's expected output.src/install/lockfile/Package/WorkspaceMap.zig (1)
229-229: Correct implementation of symlink following for workspace glob patterns.The
follow_symlinksparameter (7th positional argument) is correctly set totrue, enabling the GlobWalker to discover workspace packages that are symlinked to external directories. Allocator management is proper withdefer walker.deinit(false)cleanup. All Zig guidelines are followed:@importstatements are at the bottom of the file, and arena allocation includes proper defer reset.
| // Create a symlinked package pointing to the real package but with a different name | ||
| mkdirSync(join(String(monorepoDir), "packages", "pkg-alias")); | ||
| symlinkSync( | ||
| join(String(monorepoDir), "packages", "real-pkg", "package.json"), | ||
| join(String(monorepoDir), "packages", "pkg-alias", "package.json"), | ||
| ); | ||
|
|
||
| // Also create a separate workspace package.json for the alias | ||
| // Actually, let's test a cleaner scenario - symlink to an external package in packages/ | ||
| using externalPkgDir = tempDir("external-alias-pkg", { | ||
| "package.json": JSON.stringify({ | ||
| name: "pkg-alias", | ||
| version: "2.0.0", | ||
| }), | ||
| }); | ||
|
|
||
| // Remove the manually created directory and create proper symlink | ||
| await Bun.$`rm -rf ${join(String(monorepoDir), "packages", "pkg-alias")}`; | ||
| symlinkSync(String(externalPkgDir), join(String(monorepoDir), "packages", "pkg-alias")); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove dead code and avoid shell commands.
Lines 68-73 create a directory and symlink that are immediately removed on line 85. This appears to be leftover from an earlier iteration. Additionally, line 85 uses a shell command which should be avoided per coding guidelines.
♻️ Proposed cleanup
- // Create a symlinked package pointing to the real package but with a different name
- mkdirSync(join(String(monorepoDir), "packages", "pkg-alias"));
- symlinkSync(
- join(String(monorepoDir), "packages", "real-pkg", "package.json"),
- join(String(monorepoDir), "packages", "pkg-alias", "package.json"),
- );
-
- // Also create a separate workspace package.json for the alias
- // Actually, let's test a cleaner scenario - symlink to an external package in packages/
using externalPkgDir = tempDir("external-alias-pkg", {
"package.json": JSON.stringify({
name: "pkg-alias",
version: "2.0.0",
}),
});
- // Remove the manually created directory and create proper symlink
- await Bun.$`rm -rf ${join(String(monorepoDir), "packages", "pkg-alias")}`;
+ // Create symlink to external package
symlinkSync(String(externalPkgDir), join(String(monorepoDir), "packages", "pkg-alias"));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Create a symlinked package pointing to the real package but with a different name | |
| mkdirSync(join(String(monorepoDir), "packages", "pkg-alias")); | |
| symlinkSync( | |
| join(String(monorepoDir), "packages", "real-pkg", "package.json"), | |
| join(String(monorepoDir), "packages", "pkg-alias", "package.json"), | |
| ); | |
| // Also create a separate workspace package.json for the alias | |
| // Actually, let's test a cleaner scenario - symlink to an external package in packages/ | |
| using externalPkgDir = tempDir("external-alias-pkg", { | |
| "package.json": JSON.stringify({ | |
| name: "pkg-alias", | |
| version: "2.0.0", | |
| }), | |
| }); | |
| // Remove the manually created directory and create proper symlink | |
| await Bun.$`rm -rf ${join(String(monorepoDir), "packages", "pkg-alias")}`; | |
| symlinkSync(String(externalPkgDir), join(String(monorepoDir), "packages", "pkg-alias")); | |
| // Create a symlinked package pointing to the real package but with a different name | |
| using externalPkgDir = tempDir("external-alias-pkg", { | |
| "package.json": JSON.stringify({ | |
| name: "pkg-alias", | |
| version: "2.0.0", | |
| }), | |
| }); | |
| // Create symlink to external package | |
| symlinkSync(String(externalPkgDir), join(String(monorepoDir), "packages", "pkg-alias")); |
🤖 Prompt for AI Agents
In @test/regression/issue/25801.test.ts around lines 68 - 86, Remove the dead
initial creation block (the first mkdirSync and the first symlinkSync that point
package.json into packages/pkg-alias) since it is immediately removed; keep only
the external tempDir creation and the final symlinkSync that links
externalPkgDir into packages/pkg-alias. Replace the shell removal call Bun.$`rm
-rf ${...}` with a native filesystem removal like
fs.rmSync(join(String(monorepoDir), "packages", "pkg-alias"), { recursive: true,
force: true }) or await fs.promises.rm(..., { recursive: true, force: true }) so
no shell command is used.
When using glob patterns like `./*` or `packages/*` in the workspaces array, symlinked directories pointing to packages outside the monorepo were not being discovered. This was because the GlobWalker was initialized with `follow_symlinks: false`. This change enables symlink following in the workspace glob walker, allowing workspace packages that are symlinks to be properly discovered and resolved. Fixes #25801 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
b5924f9 to
67e0f89
Compare
|
Addressed review feedback:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/install/lockfile/Package/WorkspaceMap.zig`:
- Line 229: The call to walker.initWithCwd(..., false, false, true, false, true)
correctly sets the follow_symlinks boolean (7th positional arg) to true so
symlinked workspace packages are discovered; update the call site in
WorkspaceMap.zig (the walker.initWithCwd invocation) to add an inline comment
annotating each trailing boolean (or convert to named/explicit parameter usage
if supported) so the boolean sequence is self-documenting and future readers
know which argument is follow_symlinks.
In `@test/regression/issue/25801.test.ts`:
- Around line 42-49: The test unnecessarily collects proc.stdout.text() into the
unused stdout variable; remove stdout from the destructuring and drop
proc.stdout.text() from the Promise.all call so you only await
proc.stderr.text() and proc.exited (leaving stderr and exitCode variables), or
alternatively keep proc.stdout.text() but add assertions that use stdout; update
the destructuring to match the chosen option (variables stdout, stderr, exitCode
or just stderr and exitCode) to eliminate the unused variable.
📜 Review details
Configuration used: Path: .coderabbit.yaml
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.
📥 Commits
Reviewing files that changed from the base of the PR and between b5924f91b9866cc8756f8fd5f982807794235ab1 and 6c8e9fa.
📒 Files selected for processing (2)
src/install/lockfile/Package/WorkspaceMap.zigtest/regression/issue/25801.test.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts?(x): Never usebun testdirectly - always usebun bd testto run tests with debug build changes
For single-file tests, prefer-eflag overtempDir
For multi-file tests, prefertempDirandBun.spawnover single-file tests
UsenormalizeBunSnapshotto normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
UsetempDirfromharnessto create temporary directories - do not usetmpdirSyncorfs.mkdtempSync
When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Do not write flaky tests - do not usesetTimeoutin tests; instead await the condition to be met
Verify tests fail withUSE_SYSTEM_BUN=1 bun test <file>and pass withbun bd test <file>- tests are invalid if they pass with USE_SYSTEM_BUN=1
Test files must end with.test.tsor.test.tsx
Avoid shell commands likefindorgrepin tests - use Bun's Glob and built-in tools instead
Files:
test/regression/issue/25801.test.ts
test/regression/issue/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Place regression tests for specific GitHub issues in
test/regression/issue/${issueNumber}.test.tswith real issue numbers only
Files:
test/regression/issue/25801.test.ts
test/**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
port: 0in tests - do not hardcode ports or use custom random port number functions
Files:
test/regression/issue/25801.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 bd test <...test file>to run tests with compiled code changes. Do not usebun testas it will not include your changes.
Usebun:testfor files ending in*.test.{ts,js,jsx,tsx,mjs,cjs}. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, usebun bd <file>instead ofbun bd test <file>since they expect exit code 0.
Do not set a timeout on tests. Bun already has timeouts built-in.
Files:
test/regression/issue/25801.test.ts
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, be careful with allocators and use defer for cleanup
Files:
src/install/lockfile/Package/WorkspaceMap.zig
src/**/*.zig
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.zig: Use the#prefix for private fields in Zig structs, e.g.,struct {#foo: u32 };
Use Decl literals in Zig, e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };
Place@importstatements at the bottom of the file in Zig (auto formatter will handle positioning)
Never use@import()inline inside functions in Zig; always place imports at the bottom of the file or containing struct
Files:
src/install/lockfile/Package/WorkspaceMap.zig
🧠 Learnings (18)
📓 Common learnings
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Run `bun run zig:check-all` to compile Zig code on all platforms when making platform-specific changes
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22229
File: src/bundler/LinkerGraph.zig:0-0
Timestamp: 2025-09-06T03:37:41.154Z
Learning: In Bun's codebase, when checking import record source indices in src/bundler/LinkerGraph.zig, prefer using `if (import_index >= self.import_records.len)` bounds checking over `isValid()` checks, as the bounds check is more robust and `isValid()` is a strict subset of this condition.
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
test/regression/issue/25801.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Applied to files:
test/regression/issue/25801.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun bd test <...test file>` to run tests with compiled code changes. Do not use `bun test` as it will not include your changes.
Applied to files:
test/regression/issue/25801.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to test/regression/issue/*.test.ts : Place regression tests for specific GitHub issues in `test/regression/issue/${issueNumber}.test.ts` with real issue numbers only
Applied to files:
test/regression/issue/25801.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending in `*.test.{ts,js,jsx,tsx,mjs,cjs}`. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, use `bun bd <file>` instead of `bun bd test <file>` since they expect exit code 0.
Applied to files:
test/regression/issue/25801.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : For multi-file tests, prefer `tempDir` and `Bun.spawn` over single-file tests
Applied to files:
test/regression/issue/25801.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Verify tests fail with `USE_SYSTEM_BUN=1 bun test <file>` and pass with `bun bd test <file>` - tests are invalid if they pass with USE_SYSTEM_BUN=1
Applied to files:
test/regression/issue/25801.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Use `normalizeBunSnapshot` to normalize snapshot output of tests
Applied to files:
test/regression/issue/25801.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Avoid shell commands like `find` or `grep` in tests - use Bun's Glob and built-in tools instead
Applied to files:
test/regression/issue/25801.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Never use `bun test` directly - always use `bun bd test` to run tests with debug build changes
Applied to files:
test/regression/issue/25801.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Use `tempDir` from `harness` to create temporary directories - do not use `tmpdirSync` or `fs.mkdtempSync`
Applied to files:
test/regression/issue/25801.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : For single-file tests, prefer `-e` flag over `tempDir`
Applied to files:
test/regression/issue/25801.test.ts
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use `process.platform` and `process.arch` for platform detection; these values are inlined and dead-code eliminated at build time
Applied to files:
test/regression/issue/25801.test.ts
📚 Learning: 2025-10-26T01:32:04.844Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24082
File: test/cli/test/coverage.test.ts:60-112
Timestamp: 2025-10-26T01:32:04.844Z
Learning: In the Bun repository test files (test/cli/test/*.test.ts), when spawning Bun CLI commands with Bun.spawnSync for testing, prefer using stdio: ["inherit", "inherit", "inherit"] to inherit stdio streams rather than piping them.
Applied to files:
test/regression/issue/25801.test.ts
📚 Learning: 2025-11-07T17:28:51.204Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24423
File: test/js/node/path/browserify.test.js:472-475
Timestamp: 2025-11-07T17:28:51.204Z
Learning: In test files under test/js/node/path/, markovejnovic prefers functional programming patterns (e.g., reduce, map, filter chains) over imperative loops for clarity and readability.
Applied to files:
test/regression/issue/25801.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Organize unit tests by module in directories like `/test/js/bun/` and `/test/js/node/`.
Applied to files:
test/regression/issue/25801.test.ts
📚 Learning: 2026-01-05T16:32:07.551Z
Learnt from: alii
Repo: oven-sh/bun PR: 25474
File: src/bun.js/event_loop/Sigusr1Handler.zig:0-0
Timestamp: 2026-01-05T16:32:07.551Z
Learning: In Zig codebases (e.g., Bun), treat std.posix.sigaction as returning void and do not perform runtime error handling for its failure. The Zig standard library views sigaction failures as programmer errors (unreachable) because they only occur with invalid signals like SIGKILL/SIGSTOP. Apply this pattern across Zig files that call sigaction (e.g., crash_handler.zig, main.zig, filter_run.zig, process.zig) and ensure failures are not handled as recoverable errors; prefer reaching an explicit unreachable/compile-time assumption when such failures are detected.
Applied to files:
src/install/lockfile/Package/WorkspaceMap.zig
🧬 Code graph analysis (1)
test/regression/issue/25801.test.ts (1)
test/harness.ts (1)
tempDir(277-284)
🔇 Additional comments (2)
test/regression/issue/25801.test.ts (2)
10-50: Well-structured regression tests.The tests properly cover the two scenarios from issue
#25801:
- Symlinks to external directories with root-level glob patterns (
./*)- Symlinks within a packages directory with nested glob patterns (
packages/*)Good use of
tempDirwith theusingkeyword for automatic cleanup, and proper isolation of external packages outside the monorepo to verify symlink following behavior.Also applies to: 52-96
88-95: Remove unusedstdoutvariable.Same issue as the first test -
stdoutis collected but not used.♻️ Proposed fix
- const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]);⛔ Skipped due to learnings
Learnt from: CR Repo: oven-sh/bun PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-16T00:21:32.179Z Learning: Applies to **/*.test.ts?(x) : When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failureLearnt from: Jarred-Sumner Repo: oven-sh/bun PR: 24082 File: test/cli/test/coverage.test.ts:60-112 Timestamp: 2025-10-26T01:32:04.844Z Learning: In the Bun repository test files (test/cli/test/*.test.ts), when spawning Bun CLI commands with Bun.spawnSync for testing, prefer using stdio: ["inherit", "inherit", "inherit"] to inherit stdio streams rather than piping them.Learnt from: markovejnovic Repo: oven-sh/bun PR: 24417 File: test/js/bun/spawn/spawn.test.ts:903-918 Timestamp: 2025-11-06T00:58:23.965Z Learning: In Bun test files, `await using` with spawn() is appropriate for long-running processes that need guaranteed cleanup on scope exit or when explicitly testing disposal behavior. For short-lived processes that exit naturally (e.g., console.log scripts), the pattern `const proc = spawn(...); await proc.exited;` is standard and more common, as evidenced by 24 instances vs 4 `await using` instances in test/js/bun/spawn/spawn.test.ts.Learnt from: Jarred-Sumner Repo: oven-sh/bun PR: 24491 File: test/js/bun/transpiler/declare-global.test.ts:17-17 Timestamp: 2025-11-08T04:06:33.198Z Learning: In Bun test files, `await using` with Bun.spawn() is the preferred pattern for spawned processes regardless of whether they are short-lived or long-running. Do not suggest replacing `await using proc = Bun.spawn(...)` with `const proc = Bun.spawn(...); await proc.exited;`.Learnt from: CR Repo: oven-sh/bun PR: 0 File: test/CLAUDE.md:0-0 Timestamp: 2026-01-05T23:04:01.518Z Learning: Always check exit codes and test error scenarios when spawning processes in tests.Learnt from: CR Repo: oven-sh/bun PR: 0 File: test/js/node/test/parallel/CLAUDE.md:0-0 Timestamp: 2026-01-14T21:08:10.407Z Learning: Node.js compatibility tests pass by exiting with code 0
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| var cwd = bun.path.dirname(source.path.text, .auto); | ||
| cwd = if (bun.strings.eql(cwd, "")) bun.fs.FileSystem.instance.top_level_dir else cwd; | ||
| if ((try walker.initWithCwd(&arena, glob_pattern, cwd, false, false, false, false, true)).asErr()) |e| { | ||
| if ((try walker.initWithCwd(&arena, glob_pattern, cwd, false, false, true, false, true)).asErr()) |e| { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
# Find GlobWalker initWithCwd function signature
rg "fn initWithCwd" -A 2 --type zigRepository: oven-sh/bun
Length of output: 212
🏁 Script executed:
# Also check the exact context at line 229 in WorkspaceMap.zig
sed -n '225,235p' src/install/lockfile/Package/WorkspaceMap.zigRepository: oven-sh/bun
Length of output: 645
🏁 Script executed:
# Get the complete initWithCwd function signature
sed -n '/pub fn initWithCwd/,/^[[:space:]]*)/p' src/glob/GlobWalker.zig | head -20Repository: oven-sh/bun
Length of output: 387
The follow_symlinks parameter is correctly set to true at position 7 in initWithCwd().
This change enables GlobWalker to follow symlinks when scanning workspace glob patterns, fixing the issue where symlinked workspace packages outside the monorepo weren't discovered. The boolean parameter mapping is correct: the 7th positional argument corresponds to the follow_symlinks: bool parameter.
Consider adding an inline comment to clarify the boolean parameter sequence, as the positional booleans in initWithCwd are not immediately self-documenting.
🤖 Prompt for AI Agents
In `@src/install/lockfile/Package/WorkspaceMap.zig` at line 229, The call to
walker.initWithCwd(..., false, false, true, false, true) correctly sets the
follow_symlinks boolean (7th positional arg) to true so symlinked workspace
packages are discovered; update the call site in WorkspaceMap.zig (the
walker.initWithCwd invocation) to add an inline comment annotating each trailing
boolean (or convert to named/explicit parameter usage if supported) so the
boolean sequence is self-documenting and future readers know which argument is
follow_symlinks.
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| // Should not fail with "Workspace dependency 'backend' not found" | ||
| expect(stderr).not.toContain("Workspace dependency"); | ||
| expect(stderr).not.toContain("not found"); | ||
|
|
||
| // Should succeed | ||
| expect(exitCode).toBe(0); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove unused stdout variable.
The stdout variable is collected but never used in the assertions. Either remove it from the destructuring or add assertions that use it.
♻️ Proposed fix
- const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+ const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]);🤖 Prompt for AI Agents
In `@test/regression/issue/25801.test.ts` around lines 42 - 49, The test
unnecessarily collects proc.stdout.text() into the unused stdout variable;
remove stdout from the destructuring and drop proc.stdout.text() from the
Promise.all call so you only await proc.stderr.text() and proc.exited (leaving
stderr and exitCode variables), or alternatively keep proc.stdout.text() but add
assertions that use stdout; update the destructuring to match the chosen option
(variables stdout, stderr, exitCode or just stderr and exitCode) to eliminate
the unused variable.
|
Closing this PR because it has been inactive for more than 90 days. |
2 participants
Summary
workspacesarrayfollow_symlinksparameter fromfalsetotrueinWorkspaceMap.zig:229Fixes #25801
Test plan
test/regression/issue/25801.test.tsthat verifies:./*packages/*patterns are followed correctly🤖 Generated with Claude Code