feat(workflow): add engine core — execution model, graph, and node registry (Part 2) - #588
feat(workflow): add engine core — execution model, graph, and node registry (Part 2)#588kalenkevich wants to merge 7 commits into
Conversation
a49ab3f to
7ec20c0
Compare
6634dc6 to
44346d7
Compare
AmaadMartin
left a comment
There was a problem hiding this comment.
Reviewed the Part 2 delta only (base is feat/workflows_part1). The timeout cancellation in runOnce is genuinely well done — the abort-signal plumbing, the single reused aborted promise so no listener leaks per step, and the iterator.return() in finally are all correct, and node_execution_test.ts:230 proves post-deadline events are dropped rather than leaking into the next node. I also checked two things the stack made me suspicious of and both are clean: the for (;;) retry loop is bounded by maxAttempts ?? DEFAULT_MAX_ATTEMPTS (utils/retry_utils.ts:51-55), and everything the engine writes — event.output, event.actions.agentState, functionCall.args — is already in PRESERVE_KEYS (events/event.ts:359), so nothing gets key-mangled on a session round-trip. The findings below cluster on three things: a graph that is fully usable before it is validated, module-global registry state, and instanceof where this package already has a copy-safe guard pattern.
570ce49 to
a97a9ba
Compare
…gistry Part 2/9 of the feature/workflows split. Adds the strongly-connected engine core, stacked on Part 1's primitives: - base_node: BaseNode abstract class + START sentinel - node_context / node_runner: per-node execution context and the run loop (retry, timeout/cancellation, streaming) - graph + utils/graph_parser + utils/graph_validation: chain/edge parsing, routing maps, and structural validation (START reachability, duplicate names/edges, DEFAULT_ROUTE, unconditional-cycle detection) - schedule_dynamic_node: dynamic scheduling primitive used by node_context - request_input + utils/hitl_utils: human-in-the-loop request primitives consumed by BaseNode Decoupling refactor (enables the layered split): utils/workflow_graph_utils no longer statically imports the concrete node classes. buildNode/isNodeLike now consult a self-registration registry (registerNodeBuilder, registerParallelWorkerFactory); concrete node modules register at load time (Parts 3+). This breaks the engine<->nodes import cycle. Builds on Part 1's review-driven APIs: streams events via the shared AsyncQueue (not a bespoke channel), reads branches via the createSubBranch util function, and normalizes retry configs once at construction (BaseNode.preparedRetryConfig + prepareRetryConfig) so the retry loop never re-normalizes or throws mid-retry. Bundled tests (28): node_execution, graph_parser, graph_validation, plus shared test_helpers. Full core suite green (2366 tests).
Addresses the Part 2 review plus two follow-up requests (no static
methods, no `instanceof`) and support for multiple schema formats.
Review fixes:
- Graph is validated at construction (`createGraphFromEdgeItems`) instead
of leaving validation opt-in.
- Route keys are matched by string value so a node emitting '2'/'true'
can't silently miss its edge; drop the dead route-key guard branch.
- `toContent` reuses `isContent` (single predicate) and handles
JSON.stringify returning undefined.
- Retry now clears the failed attempt's `stateDelta` (in place) before
retrying; event-replay semantics documented on `executeChildNode`.
- `withBranch` builds the child context via `new InvocationContext(ic)`
(the ParallelAgent pattern) instead of a lossy double-cast spread.
- Node registry is keyed by builder id (idempotent) with explicit
priority ordering; parallel-worker factory rejects a conflicting
re-registration; single-global-registry decision documented.
- Accurate unconditional-cycle error message; engine-owned event `path`
documented; typed `InvocationAbortedError` for abort-during-backoff.
- Deduplicate the test harness onto `test_helpers` (no double casts;
real `BaseAgent` + `createSession`).
No static methods: convert `Graph.fromEdgeItems` and the `BranchPath`
statics to plain functions.
No `instanceof`: brand `BaseNode`/`Edge`/`RequestInput` and the workflow
error classes with `Symbol.for('google.adk.*')` signatures and match on
the brand (copy-safe); duck-type the retry error-name helper.
Schema support: add `SchemaLike` (Zod v3 | Zod v4 | genai `Schema`) plus
`parseWithSchema`/`toJsonSchema` in utils/schema.ts and reuse across
base_node, workflow_graph_utils, and request_input/hitl_utils.
Revert the error classes back to `instanceof Error` + name matching for their type guards, dropping the Symbol.for brands. Brand-based guards are kept for the non-error types (BaseNode, Edge, RequestInput).
… fix Adds essential unit coverage across the Part 2 surface, plus two follow-ups on the same iteration. Tests (6 new suites + extensions): - schema: parseWithSchema / toJsonSchema across Zod v3, Zod v4, and genai Schema (validation, passthrough, JSON conversion). - branch_path: fromString, append, isDescendantOf, common-prefix, and the createSubBranch / commonPrefixOf wrappers. - graph: isEdge brand, getNextPendingNodes routing (unconditional, specific, numeric/boolean string-coercion matching, route lists, fan-out, default-route precedence), createGraphFromEdgeItems validation. - workflow_graph_utils: isPlainObject / isNodeLike / buildNode and the registry (idempotency by id, priority + tie-break, parallel-worker factory conflict rejection). - base_node: isBaseNode / isContent brands and toContent across all branches (incl. circular-ref safety). - hitl: RequestInput + isRequestInput, createRequestInputEvent with responseSchema conversion, createRequestInputResponse, auth-resume. - foundations / node_execution: InvocationAbortedError guard, input-schema validation, and abort-during-retry-backoff. Refactor: helper/utility functions taking more than two arguments now accept a single destructured params object (executeChildNode, runOnce, enrichEvent, the graph_parser edge helpers, shouldRetryNode, getRetryDelaySeconds, processAuthResume); call sites updated. The fluent runNode and the Edge constructor stay positional. Fix: toContent no longer throws on an arbitrary node output — a plain object/number/boolean is serialized to text instead of being passed straight to createModelContent (which only accepts strings, Parts, or arrays of them).
9dcc111 to
b136d83
Compare
AmaadMartin
left a comment
There was a problem hiding this comment.
Re-reviewed at b136d83b. Eight of the ten findings from my earlier pass are genuinely fixed, and I verified each against the head tree rather than trusting the commit subjects:
- Graph is validated before use —
validateGraphnow runs on the construction path and populatesterminalNodeNames, so a malformed graph fails at build time instead of silently yielding an empty terminal set. instanceofis gone — replaced with brand symbols on nodes, edges andRequestInput. Every remaining occurrence of the word is a doc comment explaining why the brand is used instead, which is exactly the cross-package-copy argument from #364. The HITL path was the load-bearing one and it is now copy-safe.- Route keys match symmetrically —
String(route)on both sides, so a numeric-looking string key can no longer normalize into a value the matcher never compares equal to. The residual ambiguity ('2'vs2being indistinguishable) is documented at the type rather than left implicit, which is the right call. - Retry no longer leaks partial writes —
actions.stateDeltais cleared alongside output/route/interrupts, so a failed attempt cannot commit state. - The double cast and lossy spread in the sub-branch path are gone, and
enrichEventno longer clobbersnodeInfo.path.
CI is green on ubuntu/macOS/Windows — I checked the individual jobs rather than the rollup, since a rollup on this repo can read green when only check-changes and the CLA actually ran.
Approving, with two things I would not want lost — neither blocks this part, but both get harder to change once Parts 3-8 stack on it:
- No runtime step cap. The cycle rule permits routed cycles, and the validation doc no longer claims a conditional edge prevents an infinite loop — correct, but nothing bounds iterations at run time either. This is invisible today because the engine is not user-reachable until the runner lands in Part 5; it becomes reachable exactly when it does. A scheduler-level max-steps guard is a small localized change now and an awkward one later.
- The node registry is still process-global. The new guard against registering a conflicting
parallelWorkerFactoryremoves the worst failure mode, so this is no longer a correctness trap — but two workflows in one process still share builder state, and nothing scopes it per workflow.
Worth filing #1 against Part 5 so it is not discovered by an agent that loops.
… list
Drop the runtime node-builder registry (registerNodeBuilder /
registerParallelWorkerFactory and the id/priority/idempotency/
conflict-rejection machinery it needed) in favor of a single explicit,
statically-imported list.
- Add node_builders.ts exporting NODE_BUILDERS (ordered; first match wins)
and PARALLEL_WORKER_FACTORY. Node-type parts wire their builders in here
instead of self-registering at import time.
- buildNode / isNodeLike consult NODE_BUILDERS; buildNode uses
PARALLEL_WORKER_FACTORY. NodeBuilder is now just {match, build} (no id /
priority — order is precedence).
This removes global mutable state and import-order side effects (the
"builder only exists if the module was imported" fragility). The list is
empty in the engine-core part; concrete node types populate it in their
parts.
Replace the two infinite loops with condition-driven ones: the retry loop becomes while(!succeeded) (throw on a non-retryable failure), and the timeout drive-loop becomes while(!result.done). Behavior is unchanged.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
2. Or, if no issue exists, describe the change:
Problem:
Continuing the split of the large
feature/workflowsbranch into small, stacked, reviewable PRs. The workflow engine's core files form a tightly-connected import cycle (graph → graph_parser → workflow_graph_utils → concrete node classes → base_node → node_context → graph), which would otherwise force the entire engine and every node type to land in one giant PR.Solution:
This is Part 2 of 9 — the engine core — stacked on Part 1. It adds the execution model, the graph layer, and a decoupling refactor that breaks the cycle so each node family can ship as its own downstream PR.
Stacked on: #part1_pr_number (Part 1 — events & shared primitives). Please merge Part 1 first.
Included:
base_node.ts— theBaseNodeabstract class and theSTARTsentinel.node_context.ts/node_runner.ts— per-node execution context and the run loop (retry, timeout/cancellation, streaming viaEventChannel).graph.ts+utils/graph_parser.ts+utils/graph_validation.ts— chain/edge parsing, routing maps, and structural validation (START reachability, duplicate node names/edges,DEFAULT_ROUTErules, unconditional-cycle detection).schedule_dynamic_node.ts— the dynamic-scheduling primitivenode_contextdepends on.request_input.ts+utils/hitl_utils.ts— the human-in-the-loop request primitivesBaseNodeconsumes (the HITL processors/tools that use them arrive in Part 8).Decoupling refactor (the key change to review):
utils/workflow_graph_utils.tsno longer statically imports the concrete node classes (FunctionNode,ToolNode,ParallelWorker,LLMAgentWrapper). Instead,buildNode()/isNodeLike()consult a self-registration registry:registerNodeBuilder({match, build})— node modules register how to build themselves from aNodeLikevalue.registerParallelWorkerFactory(...)— the parallel worker registers its wrapping behavior.'START'sentinel and existingBaseNodeinstances remain built-in (no registration needed).Node modules self-register at import time (added in Parts 3+); the public workflow barrel (Part 6) imports them all, so registration is guaranteed for real usage. This breaks the engine↔nodes cycle without changing observable behavior.
Builds on Part 1's review-driven APIs: the engine streams events through the shared
AsyncQueue(Part 1 foldedEventChannelinto it), resolves branches via thecreateSubBranchutil function, and readsBaseNode.preparedRetryConfig(retry config normalized once at construction) so the run loop never re-normalizes or throws mid-retry.Intentionally deferred: the
node()user API (node.ts) lands with the first concrete node builders in Part 3; node/tool/parallel/agent node classes in Parts 3, 4, 7; thenode_api_testandrouting_testsuites move to Part 6 because they require concrete nodes and the runner.Testing Plan
Unit Tests:
Bundled tests (28):
workflow/node_execution_test.ts,workflow/graph_parser_test.ts,workflow/graph_validation_test.ts(plus sharedworkflow/test_helpers.ts). They exercise the engine using test-localBaseNodesubclasses, so they validate the core without any concrete node types registered.Typecheck is clean:
npx tsc --noEmit -p core/tsconfig.json.Manual End-to-End (E2E) Tests:
N/A — no user-facing surface yet (the
node()API and public barrel arrive in Parts 3 and 6). End-to-end coverage lands with the runner integration in Part 6.Checklist
Additional context