fix(core): avoid string-built CJS runner wrapper#7485
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the CommonJsRunner to use vm.compileFunction instead of manually wrapping code in a function string and executing it via vm.runInThisContext. This change simplifies the execution logic and removes the need for manual line and column offsets. A review comment highlights a potential compatibility issue, noting that vm.compileFunction with the importModuleDynamically option requires Node.js 16.12.0 or higher, which is more restrictive than the previous implementation and could cause runtime errors in older environments.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates the CommonJS bundle execution path to avoid constructing a wrapper function via string concatenation, switching to vm.compileFunction to satisfy CodeQL’s js/bad-code-sanitization guidance while preserving existing module scoping and dynamic import() behavior.
Changes:
- Replace string-built
(function(...) { ... })wrapper +vm.runInThisContextwithvm.compileFunction. - Adjust
preExecuteto run on the raw bundle content (no wrapper prefix).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
vm.compileFunctionwhen executing bundle outputimport()behavior while avoiding the CodeQLjs/bad-code-sanitizationpatternRelated Links