refactor(cli): replace deprecated vm2 with native node:vm runner by mbiernacik · Pull Request #2310 · dataform-co/dataform

mbiernacik · 2026-09-15T14:38:38Z

Summary

This PR replaces the deprecated and unmaintained vm2 library with native node:vm (VmRunner) across Dataform CLI, Core testing, and build targets.

Migrating to native node:vm removes an unmaintained third-party dependency, addresses deprecation warnings and known CVEs associated with vm2, and delivers substantial performance gains across compilation workflows.

Key Changes Across the Codebase

Native VmRunner Implementation (common/vm/vm_runner.ts & common/vm/BUILD):
- Implemented a standalone, reusable VmRunner under //common/vm (preventing circular layering with //cli and //testing).
- Uses node:vm.createContext and node:vm.compileFunction with scoped CommonJS require simulation.
- Enforces projectDir boundary containment (isPathContained) with an opt-in allowedExternalPaths configuration.
- Evicts failed module entries from moduleCache on throw so that errors re-throw consistently on subsequent require() calls.
- Supports configurable environment isolation via env and envAllowlist options.
- Exposes host Uint8Array and ArrayBuffer in sandbox globals to ensure cross-realm instanceof Uint8Array checks pass in protobufjs / JIT compilation.
- Memoizes resolution lookups via resolveCache and captures suppressed resolution errors as err.cause on MODULE_NOT_FOUND.
- Comprehensive unit test suite (common/vm/vm_runner_test.ts via //common/vm:tests).
CLI Compilation & JIT Worker Migration (cli/vm/compile.ts, cli/vm/jit_worker.ts):
- Migrated standard project compilation in cli/vm/compile.ts from NodeVM to VmRunner.
- Maintained compilation support for Dataform notebook actions (.ipynb, .md) via sourceExtensions.
- Migrated JIT worker compilation in cli/vm/jit_worker.ts to VmRunner.
Core Test Harness & Property Graphs (testing/run_core.ts, testing/BUILD, core/main_property_graphs_test.ts):
- Migrated testing/run_core.ts (runMainInVm) to execute tests through VmRunner.
- Cleaned up workaround logic in core/main_property_graphs_test.ts by removing the artificial vm2 empty CallSite stack workaround and unifying error assertions via asPlainGraph.
Packaging & Dependency Cleanup (package.json, yarn.lock, packages/@dataform/cli/BUILD, packages/rollup.config.js):
- Removed vm2 from package.json and yarn.lock.
- Removed vm2 from externals in packages/@dataform/cli/BUILD.
- Added vm and module to knownNodeBuiltins in packages/rollup.config.js.
Performance Benchmark (common/vm/vm_runner_benchmark.ts):
- Added a reproducible benchmark runnable via bazel run //common/vm:benchmark (~10,300 module requires/sec).

apilaskowski

Thanks for taking this on — removing vm2 is clearly the right call, and I think VmRunner is a good shape for the replacement.

Most of my inline comments are suggestions rather than objections. The one I'd genuinely flag is the module cache entry being retained when a module throws (cli/vm/vm_runner.ts:148), since that can turn a load failure into a silently incomplete compiledGraph rather than a clear error.

A few things that didn't fit on a specific line:

Security framing. The description says this "eliminates security vulnerabilities" and describes a "hermetic" runner with a "secure module resolver". node:vm doesn't really provide isolation — Node's docs are fairly explicit that it isn't a security mechanism, and vm2 existed because of that gap. If the CLI only ever compiles the invoking user's own repo on their own machine, then vm2's CVEs probably weren't reachable here either, so the security angle may not be doing much work in either direction. "Removes an unmaintained dependency and is meaningfully faster" seems like ample justification on its own. Would you consider rewording that part?
Feature list. I couldn't find implementations for three things the description mentions: execution timeouts, node:vm.Script (only vm.compileFunction is used), and script caching (moduleCache holds module exports rather than compiled scripts). Possibly planned and dropped, or I'm looking in the wrong place — either way it'd be good to bring the description back in line.
Benchmarks. Could the benchmark script land under cli/vm/? The numbers are a nice result and it'd be good to be able to re-run them. I did notice the vm2 baselines all end in .00 while the node:vm figures carry two decimals, which made me wonder whether the "before" column was measured or estimated.
Leftover vm2 references. core/session.ts:72 and :634, core/main_session_test.ts:164 and cli/index_compile_test.ts:102 still explain live workarounds in terms of "vm2's sandbox stack stripping". Since compile.ts:65-68 now says the opposite holds, is the __df_enter/__dataform_current_file file-stack machinery still needed for @dataform/core >= 3.0.57?

And lets make sure we are properly rebased on top of main branch now :)

apilaskowski · 2026-09-16T15:06:58Z

+    builtinModules: ["path"],
+    resolve: (moduleName, parentDirName) =>
+      path.join(parentDirName, path.relative(parentDirName, compileConfig.projectDir), moduleName),
+    sourceExtensions: ["js", "sql", "sqlx", "yaml", "yml", "ipynb", "md"],


ipynb and md look unrelated to the vm2 removal

Adding these two isn't mentioned in the description and doesn't have a test, and it has a couple of knock-on effects: notebook and markdown files now go through the SQLX compiler, and both extensions join allExtensions, so they participate in extension-less resolution and directory-index lookup.

Is this by accident?

Required for Dataform notebook actions (actions.yaml), which load .ipynb/.md via nativeRequire().asJson. Without registering these extensions, VmRunner tries to evaluate them as JS and throws a syntax error on "cells": [...]. Added a test.

apilaskowski · 2026-09-16T15:39:25Z

After my review I would also like @Ikolina or someone who is longer in Dataform team to also take a look here.

apilaskowski · 2026-09-17T16:05:03Z

Thanks for addressing all the previous feedback so thoroughly — moving VmRunner to //common/vm and the test coverage look great!

Just one small thing I noticed on customResolve / isPathContained (left inline), and a quick heads-up that the PR description on GitHub might not have saved when you edited it (it still lists cli/vm/vm_runner.ts and mentions timeouts/vm.Script). Otherwise this looks ready to go!

kolina · 2026-09-21T21:39:24Z

+      // Check project node_modules directory directly (e.g. @dataform/core)
+      const nodeModulesCandidate = path.resolve(this.projectDir, "node_modules", moduleName);
+      const resolvedNodeModules = this.tryResolvePath(nodeModulesCandidate);
+      if (resolvedNodeModules) {


should you also call isPathContained here?

Done. Added containment check here as well.

kolina · 2026-09-21T21:42:12Z

+          e &&
+          e.code === "MODULE_NOT_FOUND" &&
+          e.message &&
+          e.message.includes("outside of project directory")


these checks are a bit clumsy taking into account that you throw an error just above so you could store a boolean flag and check it here

Done. Moved containment check outside try/catch so we don't catch/re-throw our own error.

kolina · 2026-09-21T21:43:09Z

+        if (!this.isPathContained(resolved)) {
+          const err: any = new Error(
+            `Cannot require '${moduleName}' outside of project directory '${this.projectDir}'`,
+          );
+          err.code = "MODULE_NOT_FOUND";
+          throw err;
+        }
+        this.resolveCache.set(cacheKey, resolved);
+        return resolved;


this logic is repeated in a lot of places, could we deduplicate it?

Done. Deduplicated into helper checkContainmentAndCache().

kolina · 2026-09-21T21:45:02Z

+          e &&
+          e.code === "MODULE_NOT_FOUND" &&
+          e.message &&
+          e.message.includes("outside of project directory")
+        ) {
+          throw e;
+        }


same: let's save into boolean flag before throwing error above to avoid this complicated condition

Done. Containment check moved outside try/catch.

kolina · 2026-09-21T22:02:19Z

+            const pkg = JSON.parse(fs.readFileSync(pkgPath, "utf8"));
+            if (pkg.main) {
+              const mainPath = path.resolve(candidatePath, pkg.main);
+              const resolvedMain = this.tryResolvePath(mainPath);


can this theoretically create infinite recursion?

Good catch. Added visitedDirs cycle tracking and guarded against mainPath === candidatePath.

kolina · 2026-09-21T23:02:10Z

        source = patchOldCoreCallerFile(source);
      }
      const compiledCode = compiler(source, filePath);
      return `


If v8 preserves call sites, we could conditionally apply this file-stack shim only for old versions?

Done. Gated by semver.lt(dataformCoreVersion, "3.0.57").

kolina · 2026-09-21T23:03:02Z

  const graphError = (fileName: string, message: string, extra: object = {}) => ({
    fileName,
    message,
-    stack: `Error: ${message}${graphStackTail}`,


you don't validate graphStackTail in new test version, is it intended?

Yes — graphStackTail was vm2's mocked CallSite stub (\n at CallSite {}). Under native node:vm we get real V8 call frames, so we now assert /\n\s+at /.

kolina · 2026-09-21T23:05:21Z

+        if (e && e.code === "MODULE_NOT_FOUND") {
+          throw e;
+        }


you swallow other errors here, is it intended?

Fixed. We now suppress only MODULE_NOT_FOUND and re-throw all unexpected errors.

kolina · 2026-09-21T23:08:34Z

-      require: {
-        builtin: [],
-        context: "sandbox",
-        external: { modules: ["@dataform/*"], transitive: false },


is this intended that you don't pass these external modules in new version?

Restored. Added allowedModules option to VmRunner and passed ["@dataform/*"] here.

kolina · 2026-09-21T23:09:49Z

+        }
+      }
+    } else {
+      env = { ...process.env };


not sure if default to all environment variables is a good default (unless we already have such test)

Done. Defaulted process.env to {}.

# Conflicts: # package.json # yarn.lock

mbiernacik requested a review from a team as a code owner September 15, 2026 14:38

mbiernacik requested review from a team and apilaskowski and removed request for a team September 15, 2026 14:38

apilaskowski requested changes Sep 16, 2026

View reviewed changes

ashish10alex mentioned this pull request Sep 17, 2026

dataform compile ~4x slower since @dataform/core 3.0.61 (CLI only) — encoded graph now crosses the vm2 bridge as a host Buffer #2298

Closed

mbiernacik force-pushed the migrate-away-from-vm2 branch from 39bfbb4 to 95db3da Compare September 17, 2026 14:03

chore: migrate away from deprecated vm2 to node:vm

3fc433c

mbiernacik force-pushed the migrate-away-from-vm2 branch from 95db3da to 3fc433c Compare September 17, 2026 14:24

Format modified files with prettier

dd5318c

mbiernacik requested a review from apilaskowski September 17, 2026 14:41

apilaskowski reviewed Sep 17, 2026

View reviewed changes

Comment thread common/vm/vm_runner.ts Outdated

Comment thread cli/vm/vm_runner.ts Outdated

Enforce path containment in resolver and drop redundant resolve hooks

52b522b

kolina self-requested a review September 17, 2026 18:57

mbiernacik requested a review from apilaskowski September 18, 2026 07:43

kolina requested changes Sep 21, 2026

View reviewed changes

mbiernacik added 2 commits September 22, 2026 14:01

Address review comments on VmRunner migration

cd6a769

Merge remote-tracking branch 'origin/main' into migrate-away-from-vm2

6c1f2c9

# Conflicts: # package.json # yarn.lock

mbiernacik requested a review from kolina September 22, 2026 14:25

Conversation

mbiernacik commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Key Changes Across the Codebase

Uh oh!

apilaskowski left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

apilaskowski commented Sep 16, 2026

Uh oh!

Uh oh!

Uh oh!

apilaskowski commented Sep 17, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mbiernacik commented Sep 15, 2026 •

edited

Loading

apilaskowski left a comment •

edited

Loading