Skip to content

chore(engine): remove node:vm evaluator #25

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
albertgwo wants to merge 2 commits into
base: feat/engine-pr0b-ast-arithmetic-lru
Choose a base branch
from
Open

chore(engine): remove node:vm evaluator #25

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
33f087a
refactor(engine): replace node:vm evaluator with AST interpreter
albertgwo Mar 18, 2026
ff838eb
chore(engine): remove node:vm evaluator and related tests
albertgwo Mar 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 0 additions & 32 deletions packages/engine/src/__tests__/expressions/evaluator-security.test.ts
View file
Open in desktop

This file was deleted.

158 changes: 0 additions & 158 deletions packages/engine/src/__tests__/runner/evaluator.test.ts
View file
Open in desktop

This file was deleted.

18 changes: 10 additions & 8 deletions packages/engine/src/__tests__/runner/fixtures-edge-cases.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import { describe, it, expect, vi, beforeEach } from 'vitest'
import { runGraph } from '../../runner/walker.js'
import type { RunOptions, ExecutionContext } from '../../runner/types.js'
import type { RunOptions } from '../../runner/types.js'
import type { FlowprintDocument } from '@ruminaider/flowprint-schema'
import { evaluateExpression } from '../../runner/evaluator.js'
import { interpretExpression } from '../../expressions/interpreter.js'
import type { InterpreterContext } from '../../expressions/interpreter.js'

vi.mock('../../runner/loader.js', () => ({
loadEntryPoint: vi.fn(),
Expand All @@ -28,7 +29,7 @@ function makeOptions(overrides: Partial<RunOptions> = {}): RunOptions {
}
}

function makeContext(input: unknown, results: Record<string, unknown> = {}): ExecutionContext {
function makeContext(input: unknown, results: Record<string, unknown> = {}): InterpreterContext {
const map = new Map<string, unknown>()
for (const [k, v] of Object.entries(results)) {
map.set(k, v)
Expand Down Expand Up @@ -114,7 +115,7 @@ describe('Fixtures Edge Cases', () => {
expect(doneStep?.outcome).toBe('success')
})

it('expression timeout is configurable', async () => {
it('AST interpreter rejects unsupported expressions in switch cases', async () => {
const doc = makeDoc({
decide: {
type: 'switch',
Expand All @@ -131,16 +132,17 @@ describe('Fixtures Edge Cases', () => {
},
})

const trace = await runGraph(doc, makeOptions({ expressionTimeout: 50 }))
const trace = await runGraph(doc, makeOptions())

expect(trace.status).toBe('error')
expect(trace.error).toContain('timed out')
// The AST interpreter rejects unsupported node types like WhileStatement
expect(trace.error).toBeDefined()
})

it('evaluator returns undefined for nonexistent property on context input', () => {
it('interpreter returns undefined for nonexistent property on context input', () => {
const context = makeContext({}, {})
// Accessing a property on input that doesn't exist returns undefined (no throw)
const result = evaluateExpression('input.nonexistent', context)
const result = interpretExpression('input.nonexistent', context)

expect(result).toBeUndefined()
})
Expand Down
2 changes: 0 additions & 2 deletions packages/engine/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@ export type {
export {
runGraph,
formatTrace,
evaluateExpression,
ExpressionTimeoutError,
loadEntryPoint,
loadFixtures,
Comment on lines 20 to 24
Copy link
Copy Markdown

@baz-reviewer baz-reviewer bot Mar 19, 2026

Choose a reason for hiding this comment

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

Removing evaluateExpression and ExpressionTimeoutError from runner exports breaks existing imports — should we keep exporting them or provide documented replacements?

Finding type: Breaking Changes | Severity: 🔴 High

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

Prompt for AI Agents:

In packages/engine/src/index.ts around lines 20-24, the top-level exports removed
evaluateExpression and ExpressionTimeoutError from the public API. Restore backward
compatibility by re-adding those exports (for example: export { evaluateExpression,
ExpressionTimeoutError } from './runner/index.js') or, if those symbols were moved,
re-export them from their new module locations and ensure ExpressionTimeoutError still
exists (or provide a compatible alias/class). Also update the package exports/comments
to document any replacement path if applicable.

Heads up!

Your free trial ends tomorrow.
To keep getting your PRs reviewed by Baz, update your team's subscription

} from './runner/index.js'
Comment on lines 20 to 25
Copy link
Copy Markdown

@baz-reviewer baz-reviewer bot Mar 19, 2026

Choose a reason for hiding this comment

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

packages/engine was modified but there's no .changeset/*.md entry—should we add the required changeset per CLAUDE.md to document these package changes?

Finding type: AI Coding Guidelines | Severity: 🟠 Medium

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

Prompt for AI Agents:

In packages/engine/src/index.ts around lines 20-25, the public exports were modified
(removed evaluateExpression and ExpressionTimeoutError), but there is no .changeset
entry for packages/engine. Add a new changeset file under .changeset (e.g.
.changeset/<short-descriptive-name>.md) that names the package packages/engine,
summarizes the change (mention removal of exports and other changes in
src/rules/evaluator.ts and src/runner/walker.ts), and specifies the correct version bump
(treat removal of exported API as a breaking change—major bump—unless you confirm
it’s internal-only, otherwise use minor/patch). Also run a quick scan of all modified
files under packages/engine and include a short bullet list of affected files in the
changeset description so release notes capture the scope.

Heads up!

Your free trial ends tomorrow.
To keep getting your PRs reviewed by Baz, update your team's subscription

Expand Down
19 changes: 8 additions & 11 deletions packages/engine/src/rules/evaluator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { resolve } from 'node:path'
import { parse } from 'yaml'
import { validateRules } from '@ruminaider/flowprint-schema'
import type { ExecutionContext } from '../runner/types.js'
import { evaluateExpression } from '../runner/evaluator.js'
import { interpretExpression } from '../expressions/interpreter.js'
import {
resolveDotPath,
ruleMatches,
Expand Down Expand Up @@ -55,22 +55,20 @@ export function loadRulesFile(filePath: string, projectRoot: string): RulesDocum
/**
* Evaluate a rules document against a context.
*
* Full Node.js version: supports labeled expressions (via `node:vm`),
* ExecutionContext with node results, and expression timeouts.
* For browser usage, import from `./evaluator-browser.js` instead.
* Supports labeled expressions (via AST interpreter), ExecutionContext
* with node results. For browser usage, import from `./evaluator-browser.js`
* instead.
*
* @param doc - Parsed rules document
* @param context - Execution context with input and prior node results
* @param expressionTimeout - Timeout for labeled expression evaluation (ms)
* @returns Evaluation result with matched output(s)
*/
export function evaluateRules(
doc: RulesDocument,
context: ExecutionContext,
expressionTimeout?: number,
): RulesEvaluationResult {
const rulesContext = buildRulesContext(context)
const resolvedInputs = resolveInputs(doc.inputs, rulesContext, context, expressionTimeout)
const resolvedInputs = resolveInputs(doc.inputs, rulesContext, context)

const matchedRules: Rule[] = []

Expand Down Expand Up @@ -109,13 +107,12 @@ function buildRulesContext(context: ExecutionContext): Record<string, unknown> {
* Resolve input values from the context using declared inputs or auto-discovery.
*
* Extends the browser-safe dot-path resolution with labeled expression support
* via Node.js `node:vm`.
* via the AST interpreter.
*/
function resolveInputs(
inputs: InputDef[] | undefined,
rulesContext: Record<string, unknown>,
executionContext: ExecutionContext,
expressionTimeout?: number,
): Map<string, unknown> {
const resolved = new Map<string, unknown>()

Expand All @@ -127,8 +124,8 @@ function resolveInputs(
if (typeof input === 'string') {
resolved.set(input, resolveDotPath(input, rulesContext))
} else {
// Labeled expression — requires Node.js `node:vm`
const value = evaluateExpression(input.expr, executionContext, expressionTimeout)
// Labeled expression — evaluated via AST interpreter
const value = interpretExpression(input.expr, executionContext)
resolved.set(input.label, value)
}
}
Expand Down
Loading