feat(engine): FlowprintEngine lifecycle, CompiledFlow, and observability hooks#14
feat(engine): FlowprintEngine lifecycle, CompiledFlow, and observability hooks#14albertgwo wants to merge 2 commits intofeat/engine-pr1b-shared-walkerfrom
Conversation
Implements the engine lifecycle: register() -> load() -> execute(). FlowprintEngine provides handler registration and YAML loading with schema validation. load() pre-resolves handlers (registered, expressions, rules, entry_point, native) into an immutable CompiledFlow snapshot. CompiledFlow.execute() runs the flow through walkGraph with callbacks that dispatch to the resolved handlers. Multiple execute() calls on the same CompiledFlow are independent (no shared mutable state). Key behaviors: - Registered handlers take priority over expressions/rules/entry_points - Handler resolution is frozen at load() time (hot-reload safe) - Wait nodes throw "requires start()" in execute() mode - Observability hooks (onNodeStart, onNodeComplete, onFlowError) - Hook errors are caught and logged, never break execution
27 tests covering: - Lifecycle: register -> load -> execute happy path - YAML content detection (schema: prefix, newlines) - Schema/structural validation at load() time - Handler resolution: registered > expressions > rules > entry_points - Handler override logging (with override: true suppression) - No-handler error at load() time - ExecutionResult shape (output, trace, outcome) - Trace NodeExecutionRecord field verification - Independent state across concurrent execute() calls - Hot reload: old CompiledFlow unaffected by re-register + re-load - Handler isolation: post-load engine mutation doesn't affect CompiledFlow - Switch node expression evaluation (match + default) - Wait node throws "requires start()" on execute() - Observability hooks: onNodeStart, onNodeComplete, onFlowError - Hook errors don't break execution - Trace handler discriminant values (registered, expressions, native)
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| // Engine | ||
| export { FlowprintEngine, CompiledFlow } from './engine/index.js' | ||
| export type { EngineOptions, EngineHooks, ExecutionResult } from './engine/index.js' | ||
|
|
There was a problem hiding this comment.
packages/engine now exports FlowprintEngine and CompiledFlow but this PR lacks a .changeset — should we add a .changeset describing the API/typing exports and review packages/engine/src/engine/index.ts and packages/engine/src/engine/types.ts?
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/index.ts around lines 60-63, the package-level exports were
changed to add FlowprintEngine, CompiledFlow and re-export engine types, but no
.changeset entry was added. Create a new .changeset/*.md file under packages/engine (or
the repo-level .changeset if your project uses a central folder) that documents this API
surface change: state that the engine package adds exports FlowprintEngine and
CompiledFlow and re-exports EngineOptions/EngineHooks/ExecutionResult (include their
source paths). Mark the changeset with the appropriate release type (minor if this is an
additive, non-breaking API addition) and include a brief human-readable description.
Also review packages/engine/src/engine/index.ts and packages/engine/src/engine/types.ts
for related typing/API changes and mention any notable type additions in the same
changeset.
Heads up!
Your free trial ends tomorrow.
To keep getting your PRs reviewed by Baz, update your team's subscription
| switch (handler.type) { | ||
| case 'registered': | ||
| case 'entry_point': | ||
| result = await handler.fn(ctx) | ||
| break |
There was a problem hiding this comment.
The handler execution switch in onAction (lines 104‑132) is duplicated almost verbatim inside the node.branches loop for onParallel (lines 260‑285). Both blocks walk the same handler types, evaluate expressions/rules, and default to {} for natives, which means any change to how expressions/rules/entry points run must be applied in two places. Can we extract a helper (e.g. executeResolvedAction(handler, ctx, nodeId, expressionTimeout, projectRoot) that returns the result and handles legacy context conversion) and call it from both onAction and the parallel branch helper to keep this logic 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
| switch (handler.type) { | ||
| case 'registered': | ||
| case 'entry_point': | ||
| result = await handler.fn(ctx) | ||
| break |
There was a problem hiding this comment.
registered/entry_point handlers don't persist results to ctx.state[nodeId], so results.get(nodeId) becomes undefined — should we set ctx.state[nodeId] = result as before?
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/engine/compiled-flow.ts around lines 104 to 108 inside the
buildCallbacks -> onAction handler, the 'registered'/'entry_point' switch branch invokes
handler.fn(ctx) and returns the result but does not write the result into
ctx.state[nodeId]. Modify this block to assign ctx.state[nodeId] = result after the
handler call (for both 'registered' and 'entry_point'), before constructing and emitting
the NodeExecutionRecord and returning. Ensure the new assignment mirrors how
'expressions' and 'rules' branches populate ctx.state so downstream expressions
referencing results.get(nodeId) or nodeId.field continue to work.
Heads up!
Your free trial ends tomorrow.
To keep getting your PRs reviewed by Baz, update your team's subscription
| case 'rules': { | ||
| const rulesDoc = loadRulesFile(handler.rulesFile, projectRoot) | ||
| const legacyCtx = buildLegacyContext(ctx) | ||
| const rulesResult = evaluateRules(rulesDoc, legacyCtx, expressionTimeout) | ||
| ctx.state[nodeId] = rulesResult.output | ||
| result = rulesResult.output | ||
| break |
There was a problem hiding this comment.
CompiledFlow ignores node.rules.evaluator and always runs the builtin evaluator — should we validate node.rules.evaluator like runGraph before calling loadRulesFile/evaluateRules so non-builtin evaluators fail fast?
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/engine/compiled-flow.ts around lines 120 to 126 (the rules-driven
action handler) and lines 182 to 205 (the rules-driven switch branch), the code calls
loadRulesFile/evaluateRules without validating node.rules.evaluator. Add the same
evaluator validation used in the old walker: if node.rules.evaluator (or
handler.rules.evaluator) exists and is not 'builtin', throw a descriptive Error that
includes the nodeId and the unsupported evaluator value. Do this check immediately
before calling loadRulesFile/evaluateRules so unsupported evaluators fail fast rather
than falling back to the builtin evaluator.
Heads up!
Your free trial ends tomorrow.
To keep getting your PRs reviewed by Baz, update your team's subscription
| onParallel: async ( | ||
| nodeId: string, | ||
| node: ParallelNode, | ||
| ctx: ExecutionContext, | ||
| ): Promise<unknown> => { | ||
| safeCallHook(() => hooks?.onNodeStart?.(nodeId, node.type, node.lane)) | ||
| const startedAt = performance.now() | ||
|
|
||
| const branchPromises = node.branches.map(async (branchId) => { | ||
| const branchNode = doc.nodes[branchId] | ||
| if (!branchNode) { | ||
| throw new Error(`Parallel branch node "${branchId}" not found`) |
There was a problem hiding this comment.
onParallel ignores node.join_strategy and always awaits Promise.all, should we respect node.join_strategy and use Promise.race for 'first'?
Promise.all(branchPromises) => Promise.race(branchPromises)
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/engine/compiled-flow.ts around lines 241 to 301, the onParallel
callback always awaits Promise.all(branchPromises) and constructs a resultMap, ignoring
node.join_strategy. Update the onParallel logic to respect node.join_strategy: if
node.join_strategy === 'first', use Promise.race (or an equivalent pattern) to
short-circuit when the first branch finishes, set ctx.state[nodeId] and ctx.state for
the finished branch appropriately, and produce the return value and record consistent
with the existing walker semantics; otherwise keep the current Promise.all behavior and
resultMap construction. Preserve error handling, hooks (onNodeStart/onNodeComplete), and
ensure records/output shape remain compatible with previous behavior.
Heads up!
Your free trial ends tomorrow.
To keep getting your PRs reviewed by Baz, update your team's subscription
| const branchPromises = node.branches.map(async (branchId) => { | ||
| const branchNode = doc.nodes[branchId] | ||
| if (!branchNode) { | ||
| throw new Error(`Parallel branch node "${branchId}" not found`) | ||
| } | ||
|
|
||
| const handler = resolvedHandlers.get(branchId) | ||
| if (!handler) { | ||
| throw new Error(`No resolved handler for parallel branch "${branchId}"`) | ||
| } | ||
|
|
||
| if (isActionNode(branchNode)) { |
There was a problem hiding this comment.
Branch handlers inherit the parent parallel node's ctx and thus see the parallel node id/type/lane instead of the branch's, should we instantiate a branch-specific ctx via makeCtx(branchId, branchNode.type, branchNode.lane) before each branch handler?
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/engine/compiled-flow.ts around lines 249 to 266, the onParallel
branch handler uses the parent parallel node's ctx so handler.fn and buildLegacyContext
see the parallel node identity. Change the branch handling code to create a
branch-specific context before invoking handlers: make a shallow clone of ctx but set
node: { id: branchId, type: branchNode.type, lane: branchNode.lane } (or call the
existing makeCtx helper if available) and pass this branchCtx into handler.fn,
buildLegacyContext, and evaluateRules instead of the parent ctx. Ensure ctx.state
updates still write to the parent ctx.state (or explicitly write branch results to
ctx.state[branchId]) so overall state behavior remains the same.
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: |
5e19b7c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://11fd5a06.flowprint.pages.dev |
| Branch Preview URL: | https://feat-engine-pr2-engine-lifec.flowprint.pages.dev |
Code Review FindingsCritical (90-100 confidence)1. Rules files re-read from disk at execution time — breaks CompiledFlow immutability (92)
2. Parallel branches share mutable
Important (80-89 confidence)3.
4.
5.
6.
7. No test coverage for parallel or error node execution (80)
|
1 participant
User description
Summary
Dependency
Base: `feat/engine-pr1b-shared-walker`
PR 6 of 16 in the execution engine implementation.
Test plan
Generated description
Below is a concise technical summary of the changes proposed in this PR:
Introduce FlowprintEngine lifecycle that registers handlers, parses and validates flowblueprints, resolves handlers, and returns immutable
CompiledFlowinstances configured viaEngineOptions. InstrumentCompiledFlow.execute()with observability hooks and trace collection so executions reportNodeExecutionRecords and surface lifecycle errors.CompiledFlowsnapshot with typed hooks and resulting exports.Modified files (4)
Latest Contributors(0)
CompiledFlow.execute()execution across actions, switches, parallels, errors, triggers, and terminals by invokingEngineHooks, collectingNodeExecutionRecords, and covering those flows with unit tests.Modified files (2)
Latest Contributors(0)