fix: resolve npm modules from additionalContextRoots' node_modules in nodevm#7271
Conversation
…NodeVM When shared script folders configured via additionalContextRoots have their own node_modules, the NodeVM runtime now searches those directories for npm packages instead of only checking the collection and Bruno paths.
lohit-bruno
requested review from
bijin-bruno,
helloanoop and
naman-bruno
as code owners
WalkthroughExtended the CommonJS module loader to support resolving npm modules from additional context roots. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
lohit-bruno
changed the title
additionalContextRoots' node_modules in nodevm
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/bruno-js/src/sandbox/node-vm/cjs-loader.js (1)
305-339: Transitive npm deps across distinctadditionalContextRootswon't resolve.
executeModuleInVmContext(line 333) doesn't receiveadditionalContextRootsAbsolute, and the innercreateNpmModuleRequireit creates relies purely on native Node.js hierarchy traversal. This means: ifpkg-a(resolved fromrootA/node_modules/) requirespkg-bthat is only in a differentrootB/node_modules/, it'll fail.The common case — both deps installed under the same root's
node_modules— works fine via natural traversal, so this is a rare edge case. But it's a gap worth tracking if users report it.♻️ If/when this becomes an issue — pass `additionalContextRootsAbsolute` through to `executeModuleInVmContext` and `createNpmModuleRequire`
function loadNpmModule({ moduleName, collectionPath, isolatedContext, localModuleCache, additionalContextRootsAbsolute = [] }) { ... return executeModuleInVmContext({ resolvedPath, moduleName, isolatedContext, collectionPath, - localModuleCache + localModuleCache, + additionalContextRootsAbsolute }); } function executeModuleInVmContext({ resolvedPath, moduleName, isolatedContext, collectionPath, - localModuleCache + localModuleCache, + additionalContextRootsAbsolute = [] }) { ... const moduleRequire = createNpmModuleRequire({ collectionPath, isolatedContext, currentModuleDir: moduleDir, - localModuleCache + localModuleCache, + additionalContextRootsAbsolute }); ... }Then update
createNpmModuleRequireto callloadNpmModule(instead of the nativemoduleRequire.resolve) for bare specifiers, passing theadditionalContextRootsAbsolutethrough.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-js/src/sandbox/node-vm/cjs-loader.js` around lines 305 - 339, The code currently resolves a module from additionalContextRootsAbsolute but does not pass additionalContextRootsAbsolute into executeModuleInVmContext, so createNpmModuleRequire inside that function only uses Node's native resolution and cannot find transitive deps in other context roots; fix by adding an additionalContextRootsAbsolute parameter to executeModuleInVmContext and forwarding it when called from this loader, then update createNpmModuleRequire to accept and use additionalContextRootsAbsolute and, for bare specifiers, call loadNpmModule (instead of moduleRequire.resolve) while passing additionalContextRootsAbsolute through so transitive npm dependencies across distinct context roots can be resolved.packages/bruno-js/src/sandbox/node-vm/index.spec.js (1)
244-315: Missing test for collection-over-additionalContextRoots priority and multi-root fallback.Both new tests cover the happy path, but there are two meaningful behavioural gaps:
- Priority: when the same package name is in both
collectionPath/node_modulesandadditionalRoot/node_modules, the collection version must win (as documented in the resolution-order comment). Without a test this can silently regress.- Multi-root fallback: the code iterates through multiple
additionalContextRootsAbsoluteand stops at the first hit. A test with two roots (module in the second only) would lock this in.✅ Suggested additions to the test suite
+ it('should prefer collection node_modules over additionalContextRoots node_modules', async () => { + // Same package in both roots; collection version should win + const collectionNodeModulesDir = path.join(collectionPath, 'node_modules', 'priority-pkg'); + fs.mkdirSync(collectionNodeModulesDir, { recursive: true }); + fs.writeFileSync( + path.join(collectionNodeModulesDir, 'index.js'), + 'module.exports = { source: "collection" };' + ); + + const additionalRoot = path.join(testDir, 'shared'); + fs.mkdirSync(additionalRoot); + const sharedNodeModulesDir = path.join(additionalRoot, 'node_modules', 'priority-pkg'); + fs.mkdirSync(sharedNodeModulesDir, { recursive: true }); + fs.writeFileSync( + path.join(sharedNodeModulesDir, 'index.js'), + 'module.exports = { source: "additionalRoot" };' + ); + + const script = ` + const pkg = require('priority-pkg'); + bru.setVar('source', pkg.source); + `; + + const context = { bru: { setVar: jest.fn() }, console: console }; + const scriptingConfig = { additionalContextRoots: [additionalRoot] }; + + await runScriptInNodeVm({ script, context, collectionPath, scriptingConfig }); + + expect(context.bru.setVar).toHaveBeenCalledWith('source', 'collection'); + }); + + it('should resolve npm module from second additionalContextRoot when not in first', async () => { + const root1 = path.join(testDir, 'root1'); + const root2 = path.join(testDir, 'root2'); + fs.mkdirSync(root1); + fs.mkdirSync(root2); + + // Package only in root2 + const pkgDir = path.join(root2, 'node_modules', 'root2-only-pkg'); + fs.mkdirSync(pkgDir, { recursive: true }); + fs.writeFileSync(path.join(pkgDir, 'index.js'), 'module.exports = { found: true };'); + + const script = ` + const pkg = require('root2-only-pkg'); + bru.setVar('found', pkg.found); + `; + + const context = { bru: { setVar: jest.fn() }, console: console }; + const scriptingConfig = { additionalContextRoots: [root1, root2] }; + + await runScriptInNodeVm({ script, context, collectionPath, scriptingConfig }); + + expect(context.bru.setVar).toHaveBeenCalledWith('found', true); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-js/src/sandbox/node-vm/index.spec.js` around lines 244 - 315, Add two tests to packages/bruno-js/src/sandbox/node-vm/index.spec.js using runScriptInNodeVm: (1) a priority test where you create the same package in collectionPath/node_modules and in an additionalRoot/node_modules and assert the module loaded is the collectionPath version (collection should win per resolution-order); (2) a multi-root fallback test where you pass additionalContextRoots with two roots, place the module only in the second root's node_modules, and assert it is resolved from that second root (verifying the loop over additionalContextRootsAbsolute falls back correctly). Use the existing test patterns (creating dirs/files, script string, context with bru.setVar, and scriptingConfig.additionalContextRoots) and assertions via context.bru.setVar to confirm which module was loaded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/bruno-js/src/sandbox/node-vm/cjs-loader.js`:
- Around line 305-339: The code currently resolves a module from
additionalContextRootsAbsolute but does not pass additionalContextRootsAbsolute
into executeModuleInVmContext, so createNpmModuleRequire inside that function
only uses Node's native resolution and cannot find transitive deps in other
context roots; fix by adding an additionalContextRootsAbsolute parameter to
executeModuleInVmContext and forwarding it when called from this loader, then
update createNpmModuleRequire to accept and use additionalContextRootsAbsolute
and, for bare specifiers, call loadNpmModule (instead of moduleRequire.resolve)
while passing additionalContextRootsAbsolute through so transitive npm
dependencies across distinct context roots can be resolved.
In `@packages/bruno-js/src/sandbox/node-vm/index.spec.js`:
- Around line 244-315: Add two tests to
packages/bruno-js/src/sandbox/node-vm/index.spec.js using runScriptInNodeVm: (1)
a priority test where you create the same package in collectionPath/node_modules
and in an additionalRoot/node_modules and assert the module loaded is the
collectionPath version (collection should win per resolution-order); (2) a
multi-root fallback test where you pass additionalContextRoots with two roots,
place the module only in the second root's node_modules, and assert it is
resolved from that second root (verifying the loop over
additionalContextRootsAbsolute falls back correctly). Use the existing test
patterns (creating dirs/files, script string, context with bru.setVar, and
scriptingConfig.additionalContextRoots) and assertions via context.bru.setVar to
confirm which module was loaded.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/bruno-js/src/sandbox/node-vm/cjs-loader.jspackages/bruno-js/src/sandbox/node-vm/index.spec.js
sanish-bruno
left a comment
sanish-bruno
left a comment
There was a problem hiding this comment.
PR Review
Found 2 issue(s): 0 major, 0 minor, 1 suggestion, 1 test gap.
Summary
- Clean, focused change that correctly threads
additionalContextRootsAbsolutethroughcreateCustomRequire→loadNpmModule - Resolution order (collection → context roots → Bruno) is sensible and the skip-duplicate check for collectionPath is correct
- Backward compatible via default parameter
[] - Tests cover both direct npm resolution and nested resolution (local module → npm module from context root)
| continue; | ||
| } | ||
| try { | ||
| const contextRequire = nodeModule.createRequire(path.join(contextRoot, 'package.json')); |
There was a problem hiding this comment.
Suggestion: Consider propagating additionalContextRootsAbsolute to createNpmModuleRequire (used by executeModuleInVmContext at line 249) for consistent resolution at nested levels.
Currently, when an npm module loaded from a context root's node_modules internally requires another npm package, it falls back to standard Node.js resolution (walking up the directory tree). This works fine when both packages live under the same node_modules/ directory. However, if an npm module in shared/node_modules/pkg-a requires a package that only exists in the collection's node_modules/, it would fail to resolve.
This is an edge case and likely acceptable for now since it mirrors standard Node.js behavior, but worth considering for future robustness.
There was a problem hiding this comment.
In which case the shared would list the dependency in it's package.json and then that would be resolved with normal node.js resolution, if that is missing, that means there was an incomplete metadata defined by the developer adding in the dependecy, which I think is a user issue as compared to an edge case that bruno should handle
| await runScriptInNodeVm({ script, context, collectionPath, scriptingConfig }); | ||
|
|
||
| expect(context.bru.setVar).toHaveBeenCalledWith('fromShared', true); | ||
| expect(context.bru.setVar).toHaveBeenCalledWith('version', '1.0.0'); |
There was a problem hiding this comment.
Test gap: Consider adding a test verifying resolution precedence when the same module name exists in both the collection's node_modules and a context root's node_modules. The code correctly checks collection first (line 296-303 of cjs-loader.js), but a test would lock in this behavior.
Suggested test:
it('should prefer collection node_modules over additionalContextRoots node_modules', async () => {
const additionalRoot = path.join(testDir, 'shared');
fs.mkdirSync(additionalRoot);
// Same module name in both locations with different values
const collectionPkg = path.join(collectionPath, 'node_modules', 'priority-pkg');
fs.mkdirSync(collectionPkg, { recursive: true });
fs.writeFileSync(path.join(collectionPkg, 'index.js'), 'module.exports = { source: "collection" };');
const sharedPkg = path.join(additionalRoot, 'node_modules', 'priority-pkg');
fs.mkdirSync(sharedPkg, { recursive: true });
fs.writeFileSync(path.join(sharedPkg, 'index.js'), 'module.exports = { source: "shared" };');
const script = `
const pkg = require('priority-pkg');
bru.setVar('source', pkg.source);
`;
const context = { bru: { setVar: jest.fn() }, console };
const scriptingConfig = { additionalContextRoots: [additionalRoot] };
await runScriptInNodeVm({ script, context, collectionPath, scriptingConfig });
expect(context.bru.setVar).toHaveBeenCalledWith('source', 'collection');
});There was a problem hiding this comment.
I agree with the addition of this test, in any case if the behavior changes in the future this would be a decent guardrail to have.
|
LGTM, other than #7271 (comment) |
3 participants
Description
When shared script folders configured via additionalContextRoots have their own node_modules, the NodeVM runtime now searches those directories for npm packages instead of only checking the collection and Bruno paths.
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit
New Features
Tests