refactor(engine): shared walkGraph core + runner migration#13
refactor(engine): shared walkGraph core + runner migration#13albertgwo wants to merge 13 commits intofeat/engine-pr1a-walker-typesfrom
Conversation
…reserved IDs) - Add assertWithinProject() utility to prevent path traversal attacks - Use it in loadEntryPoint() and loadRulesFile() before resolving paths - Add YAML parse limits (maxAliasCount: 100, schema: core) to prevent YAML bombs - Add reserved node ID validation (input, state, Math, node, output) in structural validation
…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.
…ty validation Add data_class array (pii, financial, credentials, internal) to all node types and lanes for trace redaction policy. Add expressions object to action nodes for engine-native transforms. Enforce mutual exclusivity: action nodes cannot have more than one of expressions, rules, or entry_points. Update serializer key ordering for deterministic output and add dedicated lane serializer to handle data_class arrays.
- Path containment: traversal attacks, absolute paths, prefix-matching edge cases - Reserved node IDs: all 5 reserved names rejected, non-reserved names accepted - YAML parsing limits: excessive alias count rejected, within-limit accepted
…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.
…sivity Cover schema validation, structural validation, serialization round-trips, and key ordering for the new data_class and expressions fields. 23 new test cases across validation and serialization.
…r1b-shared-walker
…r1b-shared-walker
Generic, callback-driven graph walker that centralizes traversal logic (chain-following via next pointers), context management (flat-merge after each node), compensation stack (LIFO), AbortSignal checking, and action error -> error node routing. Delegates node-type-specific behavior to pluggable WalkGraphCallbacks.
Rewrite runner/walker.ts as a thin adapter over the generic walkGraph. The adapter wires the existing Node.js evaluator, entry point loader, and rules engine into WalkGraphCallbacks. Node results are stored both flat-merged (for forward compat) and under node IDs (for backward compat with expression evaluator references like "nodeId.field").
Add 15 new walker-level tests covering traversal order, flat merge behavior (including last-writer-wins conflicts), AbortSignal, switch traversal, terminal handling, trigger->action->terminal flow, context management, error routing, compensation LIFO, and onStep trace collection. Update 3 existing rules tests to match new output shape (node-keyed + flat-merged).
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| // Expressions | ||
| export { parseExpression, validateExpressions } from './expressions/index.js' | ||
| export { | ||
| parseExpression, | ||
| clearParseCache, | ||
| validateExpressions, | ||
| interpretExpression, | ||
| InterpreterError, | ||
| LRUCache, | ||
| } from './expressions/index.js' |
There was a problem hiding this comment.
PR exposes new public exports in packages/engine and changes packages/schema but lacks a .changeset — should we add a .changeset/*.md outlining the engine and schema API changes and release notes?
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 1-9, this hunk exposes new public exports
(parseExpression, clearParseCache, validateExpressions, interpretExpression,
InterpreterError, LRUCache and related types) but no .changeset file was added. Add a
.changeset/<short-descriptive-name>.md file that documents the public API changes for
packages/engine and packages/schema: list the newly exported functions/types
(parseExpression, clearParseCache, interpretExpression, InterpreterError, LRUCache,
InterpreterContext, walkGraph, assertWithinProject, WalkGraphCallbacks,
CompensationEntry, and any added schema fields like data_class and expressions), state
the release impact (feature vs. breaking) for each package, and include brief release
notes per the CLAUDE.md Changesets for Package Changes guidelines. Ensure the changeset
frontmatter settings (packages affected and bump type) are correct and commit the new
.changeset file with the PR.
Heads up!
Your free trial ends tomorrow.
To keep getting your PRs reviewed by Baz, update your team's subscription
| const input = | ||
| options.input && typeof options.input === 'object' && !Array.isArray(options.input) | ||
| ? (options.input as Record<string, unknown>) | ||
| : ({} as Record<string, unknown>) |
There was a problem hiding this comment.
runGraph coerces non-object options.input to {} — should we pass options.input through unchanged, preserving arrays and primitives, and audit other ctx.input handlers?
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/runner/walker.ts around lines 47 to 50, the runGraph input
normalization coerces any non-object/array input to an empty object in the `runGraph`
function. Replace this coercion by passing options.input through unchanged (e.g. const
input = options.input) or by preserving arrays/primitives instead of replacing them with
{} so action entry points receive the original value. After that, review
buildLegacyContext and any uses of ctx.input to ensure they do not assume input is
always a Record<string, unknown> and adjust typing/handling accordingly (keep behavior
compatible with evaluator/rules engine).
Heads up!
Your free trial ends tomorrow.
To keep getting your PRs reviewed by Baz, update your team's subscription
| try { | ||
| // Rules-driven action | ||
| if (node.rules) { | ||
| if (node.rules.evaluator && node.rules.evaluator !== 'builtin') { | ||
| throw new Error( | ||
| `Action node "${nodeId}" uses unknown evaluator "${node.rules.evaluator}". Only "builtin" is supported.`, | ||
| ) | ||
| } | ||
|
|
||
| const rulesDoc = loadRulesFile(node.rules.file, options.projectRoot) | ||
| const legacyCtx = buildLegacyContext(ctx) | ||
| const rulesResult = evaluateRules(rulesDoc, legacyCtx, options.expressionTimeout) |
There was a problem hiding this comment.
runGraph ignores action nodes with only expressions, should we add an expressions-only branch to evaluate the expressions and return/store the result?
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/runner/walker.ts around lines 63 to 94, the onAction handler in
runGraph throws when an Action node has neither rules nor entry_points, but the schema
permits action nodes with only an expressions field. Add an expressions-only branch
between the rules branch and the entry_point logic: if node.expressions exists, build
legacyCtx via buildLegacyContext(ctx), evaluate each expression with
evaluateExpression(expr, legacyCtx, options.expressionTimeout) into an output object,
set ctx.state[nodeId] = output, call recordStep with node_id, type: 'action', status:
'completed', duration_ms and next: node.next, and return the output. This prevents
throwing for expressions-only actions and mirrors the existing rules branch behavior.
Heads up!
Your free trial ends tomorrow.
To keep getting your PRs reviewed by Baz, update your team's subscription
| // Evaluate input expressions if present | ||
| let args: unknown | ||
| if (node.inputs) { | ||
| const evaluated: Record<string, unknown> = {} | ||
| const legacyCtx = buildLegacyContext(ctx) | ||
| for (const [key, expr] of Object.entries(node.inputs)) { | ||
| evaluated[key] = evaluateExpression(expr, legacyCtx, options.expressionTimeout) |
There was a problem hiding this comment.
The if (node.inputs) block in onAction (lines 97‑108) duplicates the exact same logic later inside the parallel branch handler (lines 257‑269): build a legacy context, iterate Object.entries(node.inputs) and evaluate each expression before calling the entry point. If we ever need to tweak input evaluation (e.g. add caching, context data, timeout handling, or logging) we would have to update both spots. Can we extract a shared helper (e.g. evaluateNodeInputs(inputs, ctx, expressionTimeout)) and reuse it in both the main action flow and the parallel branch so the expression evaluation stays centralized?
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
| if (key === 'data_class' && Array.isArray(value)) { | ||
| const seq = new YAMLSeq() | ||
| for (const item of value as string[]) { | ||
| seq.add(createScalar(item)) | ||
| } |
There was a problem hiding this comment.
data_class arrays are serialized by hand twice: the loop that builds a YAMLSeq in serializeLanes (lines 134‑139) is verbatim repeated later inside serializeNode (lines 164‑169). If we ever need to change how data_class values are quoted, filtered, or validated, we now have to touch both sections and risk ending up with inconsistent output. Could we extract a helper such as serializeStringArrayField(nodeMap, 'data_class', value) or reuse the same helper from both callers so the YAML sequence logic is defined in one place instead of being duplicated?
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
Deploying flowprint with
|
| Latest commit: |
44c23df
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2d03d0c5.flowprint.pages.dev |
| Branch Preview URL: | https://feat-engine-pr1b-shared-walk.flowprint.pages.dev |
Code Review FindingsCritical (90-100 confidence)1. No cycle protection in
if (++stepCount > MAX_STEPS) {
throw new Error(`walkGraph exceeded maximum step count. Possible cycle.`)
}2.
const wrappedCallbacks = { ...callbacks, onStep: (record) => { trace.push(record); callbacks.onStep(record) } }Important (80-89 confidence)3.
4. Missing test coverage for cycles, multi-root graphs, empty node maps (82)
|
1 participant
User description
Summary
Dependency
Base: `feat/engine-pr1a-walker-types`
Also merges: PR 0a, 0b, 0c (Phase 0 foundation)
PR 5 of 16 in the execution engine implementation.
Test plan
Generated description
Below is a concise technical summary of the changes proposed in this PR:
Describe the runner migration to share the new
walkGraphtraversal core, wiring its action/switch/parallel/wait/error handling callbacks and security boundary checks so the historicrunGraphloop can be removed while preserving evaluator, rules, and compensation wiring. Summarize the expression tooling by introducing the browser-safe interpreter, cached parser, and schema extensions for arithmetic support plus data-class metadata and validation guards to keep security and serialization consistent.Modified files (2)
Latest Contributors(1)
walkGraph, its exported callbacks (WalkGraphCallbacks,CompensationEntry), and the runner adapter now own traversal, state merging, step tracing, compensation, error routing, and AbortSignal handling, while the runner wiring (runGraph, loader, rules evaluator, exports, and security helpers/tests) just feeds callbacks and enforcesassertWithinProject.Modified files (11)
Latest Contributors(1)
LRUCachebacks parse-result caching and is exercised by cache tests,parseExpressionnow caches/clears results,interpretExpressionwalks an acorn AST via allowlisted operators/methods, and the exports/tests surface the interpreter/context objects so runtime expression evaluation (including Math and node results) stays browser-safe.Modified files (11)
Latest Contributors(0)
data_classmetadata everywhere, exposeexpressionson action nodes, enforce node-ID reservations, refresh YAML serialization ordering, and validate via new tests so lanes/nodes can carry data classification labels and express expression-only actions without conflicting with rules/entry points.Modified files (5)
Latest Contributors(1)