chore(engine): remove node:vm evaluator#25
chore(engine): remove node:vm evaluator#25albertgwo wants to merge 2 commits intofeat/engine-pr0b-ast-arithmetic-lrufrom
Conversation
Switch walker.ts and rules/evaluator.ts from the node:vm-based evaluateExpression to the browser-safe AST interpretExpression. This removes the runtime dependency on node:vm for expression evaluation while maintaining identical behavior for supported expression syntax.
Delete runner/evaluator.ts (node:vm sandbox), its test suite (evaluator.test.ts, evaluator-security.test.ts), and remove evaluateExpression/ExpressionTimeoutError from public exports. The AST interpreter now handles all expression evaluation.
Deploying flowprint with
|
| Latest commit: |
ff838eb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8b4befd7.flowprint.pages.dev |
| Branch Preview URL: | https://feat-engine-pr13-delete-node.flowprint.pages.dev |
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| const rulesDoc = loadRulesFile(node.rules.file, options.projectRoot) | ||
| const rulesResult = evaluateRules(rulesDoc, context, options.expressionTimeout) | ||
| const rulesResult = evaluateRules(rulesDoc, context) | ||
| context.results.set(nodeId, rulesResult.output) | ||
|
|
There was a problem hiding this comment.
RunOptions.expressionTimeout is ignored by the new interpreter — should we enforce it in interpretExpression/evaluateRules or remove/deprecate the option?
interpretExpression(...) => interpretExpression(..., options.expressionTimeout)
Finding type: Breaking Changes | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
In packages/engine/src/runner/walker.ts around lines 159 to 162, the call to
evaluateRules(rulesDoc, context) no longer passes the RunOptions.expressionTimeout so
expression evaluation has no execution bound. Reintroduce timeout enforcement by
updating the evaluator/interpreter APIs to accept a timeout parameter and pass
options.expressionTimeout (use a default of 1000 ms if undefined) from walker.ts;
specifically, change calls at the action rules evaluation (around lines 159-162), switch
rules evaluation (around lines 257-263), per-case interpretExpression calls (around line
302), and parallel branch input evaluation (around line 368) to pass the timeout. Also
update the imported function signatures and any callers/tests/CLI forwarding to ensure
the option is honored; alternatively, if you prefer to remove the option, deprecate and
remove RunOptions.expressionTimeout and update CLI help/forwarding consistently.
Heads up!
Your free trial ends tomorrow.
To keep getting your PRs reviewed by Baz, update your team's subscription
| if (branchNode.inputs) { | ||
| const evaluated: Record<string, unknown> = {} | ||
| for (const [key, expr] of Object.entries(branchNode.inputs)) { | ||
| evaluated[key] = evaluateExpression(expr, context, options.expressionTimeout) | ||
| evaluated[key] = interpretExpression(expr, context) | ||
| } | ||
| args = evaluated |
There was a problem hiding this comment.
The input-evaluation loop is duplicated, should we extract an evaluateInputs(inputs, ctx) helper and reuse it?
const evaluateInputs = (inputs, ctx) => Object.fromEntries(Object.entries(inputs).map(([k, expr]) => [k, interpretExpression(expr, ctx)]));
Finding type: Conciseness | Severity: 🟢 Low
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
In packages/engine/src/runner/walker.ts around lines 365 to 370 (and also update the
identical loop at ~lines 183 to 189 in executeAction), the code duplicates the logic
that evaluates input expressions by calling interpretExpression for each key. Extract a
small helper function in this file, for example: const evaluateInputs = (inputs:
Record<string, string>, ctx: ExecutionContext) => { const out: Record<string, unknown> =
{}; for (const [k, expr] of Object.entries(inputs)) out[k] = interpretExpression(expr,
ctx); return out; }; Then replace both duplicated loops with a call to
evaluateInputs(node.inputs, context) (or branchNode.inputs) so both executeAction and
executeParallel use the single helper and maintain identical behavior.
Heads up!
Your free trial ends tomorrow.
To keep getting your PRs reviewed by Baz, update your team's subscription
| export { | ||
| runGraph, | ||
| formatTrace, | ||
| evaluateExpression, | ||
| ExpressionTimeoutError, | ||
| loadEntryPoint, | ||
| loadFixtures, |
There was a problem hiding this comment.
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
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
| export { | ||
| runGraph, | ||
| formatTrace, | ||
| evaluateExpression, | ||
| ExpressionTimeoutError, | ||
| loadEntryPoint, | ||
| loadFixtures, | ||
| } from './runner/index.js' |
There was a problem hiding this comment.
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
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
Code Review FindingsVerification Summary
Important (80-89 confidence)1.
OverallClean PR. Migration is complete and thorough. Merge-ready after addressing the dead option cleanup. |
1 participant
User description
Summary
Dependency
Base: `feat/engine-pr0b-ast-arithmetic-lru` (only needs AST interpreter)
PR 17 of 16 (cleanup) in the execution engine implementation.
Test plan
Generated description
Below is a concise technical summary of the changes proposed in this PR:
Replace the
node:vm-based evaluator with the AST interpreter throughout the walker and rules evaluation flows, removing the old sandboxed evaluator from the runner API. Update the walker and rules loader to callinterpretExpression, drop the exportedevaluateExpression/ExpressionTimeoutError, and rely on the interpreter context for guarded expression handling.interpretExpression, so walker inputs/conditions and rules inputs now resolve through the AST interpreter instead of the removednode:vmsandbox.Modified files (3)
Latest Contributors(1)
Modified files (5)
Latest Contributors(1)