Skip to content

install: follow symlinks when a workspace glob walks the member directories - #41567

Open
robobun wants to merge 3 commits into
mainfrom
robobun/1f8fc81c/workspace-glob-symlink
Open

robobun wants to merge 3 commits into
mainfrom
robobun/1f8fc81c/workspace-glob-symlink

Conversation

@robobun

@robobun robobun commented Sep 6, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • A workspace member that is a symlink to a directory elsewhere is not matched by a glob in workspaces (packages/*). A workspace:* dependency on it fails: Workspace dependency "s" not found on 1.4.2, s@workspace:* failed to resolve on 1.4.3-canary. Listing the member path explicitly ("packages/s") works. Reported in Workspace packages with symlinks are not discovered during bun install #25801.
  • Cause: the glob walk in WorkspaceMap (src/install/lockfile/Package/WorkspaceMap.rs:438) constructs the GlobWalker with follow_symlinks off, so a wildcard segment skips a symlinked directory.

Fix

  • Pass follow_symlinks: true. The walker already follows a symlink that a literal pattern segment names, so this only extends the same behavior to wildcard segments.
  • Safe against loops: GlobWalker keeps the chain of followed links and skips a target that is already on it (is_followed_link_cycle).
  • A member reached through a symlink is recorded under its path in packages/ (packages/s), the same as an explicit entry for that path.
  • Verified: test/cli/install/bun-workspaces.test.ts, one new case per linker, both fail on stock bun. The rest of that file (84 tests) and bad-workspace.test.ts pass.

Background

  • workspaces entries are split into literal paths and glob patterns. Literal paths are joined and read directly. Patterns run through bun_glob::GlobWalker with <pattern>/package.json, and each match becomes a member.
  • GlobWalker::init_with_cwd takes follow_symlinks and error_on_broken_symlinks. With the first off, a symlink under a wildcard is reported as a file and never descended into.
Notes

Repro on 1.4.3-canary (f42e980):

mkdir -p real/s packages/b
echo '{ "name": "mono", "private": true, "workspaces": ["packages/*"] }' > package.json
echo '{ "name": "s", "version": "1.0.0" }' > real/s/package.json; ln -s ../real/s packages/s
echo '{ "name": "b", "version": "1.0.0", "dependencies": { "s": "workspace:*" } }' > packages/b/package.json
bun install   # error: s@workspace:* failed to resolve

With the fix, bun.lock lists packages/s under workspaces, the hoisted linker links node_modules/s, and the isolated linker links packages/b/node_modules/s.

Two things are unchanged and out of scope here:

  • The "Searched in" line of the not-found error prints the dependency's version spec (./* for workspace:*), not the workspaces globs.
  • A symlink in packages/ that points back at the root (packages/loop -> ..) now matches packages/loop/package.json, so the root appears once more as a member named after itself. An explicit "packages/loop" entry has the same effect today.

#25995 was an earlier PR for #25801 with the same one-line change. It was closed for inactivity.


no test proof · iteration 2 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/install/bun-workspaces.test.ts

…tories

A workspace member that is a symlink to a directory elsewhere was not
matched by a glob such as packages/*, so a workspace:* dependency on it
failed to resolve. Listing the member path explicitly worked. The glob
walker ran with follow_symlinks off. It now follows symlinks, with the
walker's existing cycle check for links back into the chain.
@coderabbitai

coderabbitai Bot commented Sep 6, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

On-demand reviews are free for the next 14 days. After that, they cost $0.25 per reviewed file.

Or wait 15 seconds for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: 3404b037-839d-4b3d-af03-e8b39a9c9e15

📥 Commits

Reviewing files that changed from the base of the PR and between ae7b8f4 and 5430abc.

📒 Files selected for processing (2)
  • src/install/lockfile/Package/WorkspaceMap.rs
  • test/cli/install/bun-workspaces.test.ts

Comment @coderabbitai help to get the list of available commands.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread test/cli/install/bun-workspaces.test.ts Outdated
Comment thread src/install/lockfile/Package/WorkspaceMap.rs
@robobun

robobun commented Sep 6, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 7:03 AM PT - Sep 6th, 2026

❌ @robobun, your commit 5430abc has 4 failures in Build #111169 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 41567

That installs a local version of the PR into your bun-41567 executable, so you can run:

bun-41567 --bun

Comment thread src/install/lockfile/Package/WorkspaceMap.rs

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review completed

Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.

@robobun

robobun commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator Author

Status: the new tests pass on every lane. The red lanes in the last two runs are unrelated to this change: the binary-size check (it compares against a newer main than this branch is based on), test-crypto-dh-leak.js (fails on main), and one-off failures in import-meta.test.js and parallel.test.ts that are reported for main triage.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants