Stop vendored runtimes leaking into a workspace's shared node_modules - #18
Merged
Merged
Conversation
`BunScalaJSModule.bunInstall` merged vendored node_modules into `dest/node_modules` unconditionally -- including after the workspace branch had symlinked that path to the shared workspace install's Task.dest. The merge therefore wrote into another task's output, shared by every member of the workspace. - `mergeFromClasspathEntry` refuses a symlinked destination root and names the task the directory belongs to. On its own this turns a silent cross-task write into a loud failure. - The merge moved into the non-workspace branch, and workspace members with a vendored-runtime dependency now fail with an actionable message instead. Sharing one node_modules across members is the point of a workspace, so merging per-member was never going to be right; hoisting vendored trees to the workspace install is a feature, not a bug fix, and is left for later. Only reachable with a published vendored-runtime dependency -- mergeFrom- ClasspathEntry no-ops unless the entry carries META-INF/bun/node_modules -- which is why no fixture caught it. Unit 70 passing, integration 41 passing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Stack 5/N. Base is
agent/bun-dep-interpolator(#17), notmain.The bug
BunScalaJSModule.bunInstallranmergeVendoredNodeModules(…, dest / "node_modules")unconditionally — including after the workspace branch had already symlinked that path to the shared workspace install'sTask.dest.So the merge wrote through the symlink into another task's output directory, one shared by every other member of the workspace. Silent cross-task mutation, and non-deterministic depending on which member ran first.
The fix
mergeFromClasspathEntryrefuses a symlinked destination root, naming the task the directory actually belongs to. On its own this converts a silent corruption into a loud, diagnosable failure.On the second point: sharing one
node_modulesacross members is the point of a workspace, so merging per-member was never going to be right. Hoisting vendored trees up to the workspace install is the semantically correct behavior, but that's a feature rather than a bug fix — deliberately left out of this PR.Why nothing caught it
BunVendoredNodeModules.mergeFromClasspathEntryno-ops unless the classpath entry carriesMETA-INF/bun/node_modules, and no fixture combines a vendored-runtime dependency with a workspace member.The new unit test asserts both halves: the merge throws, and nothing leaked into the shared directory.
Verification
Unit 70 passing, integration 41 passing.
🤖 Generated with Claude Code