feat(engine): AST interpreter with arithmetic ops and LRU cache#10
feat(engine): AST interpreter with arithmetic ops and LRU cache#10
Conversation
…e cache Add +, -, *, /, % to the expression allowlist so arithmetic expressions are accepted by the parser. Introduce a browser-safe AST interpreter that walks parsed acorn ASTs without node:vm, supporting all allowlisted operators, methods, and Math members. Replace unbounded parse cache with a 1000-entry LRU cache to bound memory usage.
…cache Arithmetic parser tests: +, -, *, /, %, precedence (2+3*4=14), mixed arithmetic+comparison. Interpreter tests: arithmetic evaluation, comparison, logical, unary, ternary, template literals, method calls, Math functions, node results, error handling. LRU cache tests: store/retrieve, eviction at capacity, get promotes recency, capacity=1 edge case, capacity=1000. Parse cache integration tests: cached result identity, clear, isolation.
Deploying flowprint with
|
| Latest commit: |
f7b9e89
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://abf6e643.flowprint.pages.dev |
| Branch Preview URL: | https://feat-engine-pr0b-ast-arithme.flowprint.pages.dev |
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| /** | ||
| * Browser-safe AST interpreter for flowprint expressions. | ||
| * | ||
| * Walks the acorn AST and evaluates expressions without `node:vm`. | ||
| * Only supports the allowlisted AST types and operators. | ||
| */ | ||
| import * as acorn from 'acorn' | ||
| import { | ||
| ALLOWED_BINARY_OPS, | ||
| ALLOWED_LOGICAL_OPS, | ||
| ALLOWED_UNARY_OPS, | ||
| ALLOWED_METHODS, |
There was a problem hiding this comment.
packages/engine includes package-level changes but lacks a .changeset/*.md entry — should we add the required changeset per CLAUDE.md?
Finding type: AI Coding Guidelines | Severity: 🟢 Low
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
In packages/engine/src/expressions/interpreter.ts around lines 1 to 15, a new
interpreter API and exports were introduced but no .changeset entry was added. Create a
new .changeset/<short-descriptive-name>.md file that names the affected package
(packages/engine), describes the change (adds a browser-safe flowprint interpreter API
and new exports), and specifies the release type (patch/minor/major — choose minor if
this is a new public API, otherwise patch). Include a one-sentence summary and any
migration notes (e.g., new exported InterpreterError and interpretExpression), save the
file, and ensure it follows the repo's CLAUDE.md changeset format so CI recognizes the
package-level change.
Heads up!
Your free trial ends tomorrow.
To keep getting your PRs reviewed by Baz, update your team's subscription
| * Build a frozen Math object with only allowlisted methods/constants. | ||
| */ | ||
| function buildSafeMath(): Readonly<Record<string, unknown>> { | ||
| const safeMath: Record<string, unknown> = {} | ||
| for (const key of ALLOWED_MATH_MEMBERS) { | ||
| safeMath[key] = Math[key as keyof typeof Math] | ||
| } | ||
| return Object.freeze(safeMath) | ||
| } |
There was a problem hiding this comment.
buildSafeMath is reimplemented verbatim in this interpreter (lines 26‑34) and again in packages/engine/src/runner/evaluator.ts (lines 12‑20). Both constructors iterate over ALLOWED_MATH_MEMBERS, pack them into a new object, and freeze it, so any later change to which Math members are allowed or how the object is built must be made twice. Could we extract this into a shared helper (e.g. expressions/safeMath.ts exporting both buildSafeMath/SAFE_MATH or move it into allowlist.js) and import it from both places? That would keep the Math sanitization logic in one place, making updates (new constants, stricter freezing, etc.) happen once instead of needing identical edits in two files.
Finding type: Code Dedup and Conventions | Severity: 🟢 Low
Want Baz to fix this for you? Activate Fixer
Heads up!
Your free trial ends tomorrow.
To keep getting your PRs reviewed by Baz, update your team's subscription
| export function interpretExpression(source: string, context: InterpreterContext): unknown { | ||
| const ast = acorn.parseExpressionAt(source, 0, { ecmaVersion: 2022 }) | ||
| return evaluate(ast as any, context) |
There was a problem hiding this comment.
interpretExpression doesn't check for trailing tokens while parseExpression rejects them — should we throw when ast.end doesn't consume the whole source so the interpreter mirrors the parser?
Finding type: Logical Bugs | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
In packages/engine/src/expressions/interpreter.ts around lines 55 to 57, the
interpretExpression function parses the source but does not reject trailing tokens.
Modify interpretExpression to compute the parsed node end (ast.end) and compare it to
source.trimEnd().length; if they differ, throw an InterpreterError indicating unexpected
trailing tokens. This ensures the interpreter rejects inputs with extra content just
like the parser does.
Heads up!
Your free trial ends tomorrow.
To keep getting your PRs reviewed by Baz, update your team's subscription
| export function interpretExpression(source: string, context: InterpreterContext): unknown { | ||
| const ast = acorn.parseExpressionAt(source, 0, { ecmaVersion: 2022 }) | ||
| return evaluate(ast as any, context) |
There was a problem hiding this comment.
interpretExpression lets acorn.parseExpressionAt throw raw SyntaxError — should we catch it and rethrow an InterpreterError?
Finding type: Logical Bugs | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
In packages/engine/src/expressions/interpreter.ts around lines 55 to 57, the
interpretExpression function directly calls acorn.parseExpressionAt and does not handle
parse-time failures. Wrap the parseExpressionAt call in a try/catch that catches any
thrown Error (including acorn SyntaxError) and rethrow a new InterpreterError with a
clear message that includes the original error message (and optionally location) before
returning evaluate(ast, context). Ensure TypeScript types are preserved and that
interpreter behavior is unchanged for successful parses.
Heads up!
Your free trial ends tomorrow.
To keep getting your PRs reviewed by Baz, update your team's subscription
Code Review FindingsCritical (90-100 confidence)1.
const ast = acorn.parseExpressionAt(source, 0, { ecmaVersion: 2022 })
if (ast.end < source.trimEnd().length) {
throw new InterpreterError('Unexpected content after expression')
}2. Interpreter does not enforce
if (!ALLOWED_AST_TYPES.has(type)) {
throw new InterpreterError(`Disallowed AST node type: ${type}`)
}3.
Important (80-89 confidence)4.
5. Module-level
6.
|
1 participant
User description
Summary
node:vm)+,-,*,/,%,**), comparisons, logical opsDependency
Base:
main(independent of PR 0a)PR 2 of 16 in the execution engine implementation.
Test plan
Generated description
Below is a concise technical summary of the changes proposed in this PR:
Enable safer flowprint expression execution by adding a browser-safe AST interpreter that uses the allowlist and re-exports
interpretExpression,InterpreterError, andInterpreterContextso the engine can evaluate arithmetic/logical expressions withoutnode:vm. Introduce a shared LRU-backed parse cache exposed throughparseExpression/clearParseCacheso repeated parses reuse results for faster validation flows.parseExpressionwith caching/clearing helpers, verify cache semantics via dedicated tests, and expand parser suites to cover arithmetic parsing plus cache behavior.Modified files (5)
Latest Contributors(1)
Modified files (6)
Latest Contributors(1)