feat(workflow): workflow runner and public API (Part 5) - #592
feat(workflow): workflow runner and public API (Part 5)#592kalenkevich wants to merge 6 commits into
Conversation
AmaadMartin
left a comment
There was a problem hiding this comment.
Reviewed the public-API surface, the runner loop and the resume path against the files at head. Good things first: it reuses the shared AsyncQueue rather than adding a second queue, there is no any/@ts-expect-error anywhere in the diff, and I checked the new barrel names against common.ts/index.ts for collisions (none today). Three things I would not ship as-is: intentionallyNotExported hides three types that are genuinely on NodeContext's public surface, WorkflowConfig marks a strictly-required choice optional, and the fast-forward path hands user code an object cast to NodeContext that is missing every method. The rest are nits.
| export * from './tools/mcp/mcp_session_manager.js'; | ||
| export * from './tools/mcp/mcp_tool.js'; | ||
| export * from './tools/mcp/mcp_toolset.js'; | ||
| export * from './workflow/index.js'; |
There was a problem hiding this comment.
Nit. Star re-export of a ~45-symbol barrel, where the rest of this file is deliberate named exports.
export * from './workflow/index.js';Only ./common.js and the three tools/mcp/* lines use export *; everything else is export {X} / export type {Y}. With a star, the top-level @google/adk surface changes silently whenever workflow/index.ts changes, and if a name ever collides with an existing export ESM drops it from both sides with no error. I checked the current names (Node, Graph, Edge, START, RetryConfig, ErrorClass, RequestInput, NodeState, ...) against common.ts and index.ts — no collision today, so this is only about keeping the surface intentional.
Separately: the workflow barrel is added here but not to core/src/common.ts, which is what core/src/index_web.ts re-exports — so the workflow API is absent from the web entry point. Intentional? Nothing in workflow/ looks node-only.
b150359 to
0d790db
Compare
Part 5/9 of the feature/workflows split. Brings the engine together into a runnable workflow and exposes the public surface. - workflow.ts: the Workflow orchestrator — triggers, routing/fan-out, dynamic entry, and resume/fast-forward. - workflow_agent.ts: BaseAgent adapter so a Workflow runs under the ADK Runner (streams events via the shared AsyncQueue). - dynamic_node_scheduler.ts + utils/rehydration_utils.ts: dynamic scheduling and event-driven state reconstruction for resume. - node.ts: the node() user API. - workflow/index.ts + core/src/index.ts: public barrel exports; typedoc.json marks the internal AsyncQueue/ScheduleDynamicNode/NodeContextOptions as intentionally-not-exported. - register_builtin_nodes.ts: side-effect module imported by node()/workflow so the built-in Function/Tool/Parallel builders are registered even when those entry points are imported directly (not via the barrel). Adapted to Part 1's review APIs: workflow_agent uses AsyncQueue, workflow uses the commonPrefixOf util function. Tests (53): workflow, workflow_advanced, routing, parallel, dynamic_workflow, dynamic_resume, resume, runner_integration, auth_gate, hitl. Full core suite green (2444), docs:check clean, tsc clean. The LLM-agent-as-node tests (node_api, multi_agent, llm_agent) land in Part 7 with the agent builder.
Rebased Part 5 onto the current Part 4 and reconciled it with the convention changes from Parts 2-4: - Graph.fromEdgeItems -> createGraphFromEdgeItems (validates internally, so the separate validate() call is dropped). - executeChildNode now takes a single params object (workflow.ts and dynamic_node_scheduler.ts call sites updated). - Delete register_builtin_nodes.ts and its imports: with the static node-builder const list, importing node modules for side-effect registration is obsolete (node_builders.ts wires them, loaded via buildNode). - parallel_test.ts uses branchPathFromString (the static BranchPath factory is now a plain function). Also split the name-collided hitl_test.ts: the Part 3 request-input unit tests keep hitl_test.ts; Part 5's runner pause/resume tests move to hitl_flow_test.ts.
0d790db to
bf09d10
Compare
- Export SchemaLike (it is the type of the public BaseNode.inputSchema / outputSchema / stateSchema fields) from the public barrel. - Stop @link-ing internal, undocumented symbols from doc comments (parseWithSchema, toJsonSchema, isBaseNode, isEdge, REQUEST_INPUT_SIGNATURE_SYMBOL, DEFAULT_MAX_PARALLEL_WORKERS) — use plain inline code instead. `npm run docs:check` (typedoc --treatWarningsAsErrors) is now clean.
AmaadMartin
left a comment
There was a problem hiding this comment.
Re-reviewed at bf09d10a. Holding off on approval for one reason, and it is the same one as last round.
typedoc.json still carries the intentionallyNotExported suppression for AsyncQueue, ScheduleDynamicNode and NodeContextOptions, and none of the three is exported from core/src/common.ts. All three sit on the public surface of NodeContext, which this PR does export: one is a public field's type, one an optional field's type, one the constructor parameter type. So docs:check passes while a consumer still cannot name the types they need to use the class.
I am flagging this rather than filing it as a nit because of what it is: silencing the tool that detects the problem instead of fixing the problem. It is the same category as an @ts-expect-error standing in for a real type — and this is the PR where the public barrel lands, so it is the most expensive place in the stack to defer it. Exporting the three from common.ts and deleting the suppression list should be a small change; if any of them genuinely should not be public, then NodeContext needs its signature narrowed so they are not reachable from it.
Second item, non-blocking but worth deciding now rather than after release: there is still no @experimental on Workflow / WorkflowAgent / Node. A brand-new subsystem landing behind the decorator can change shape without a semver break; without it, every rough edge in Parts 6-8 becomes a compatibility commitment the moment this ships.
Also still open from last round, in rough priority order: WorkflowConfig marking edges? and dynamicEntry? both optional and throwing at runtime when neither is set (a discriminated union removes both the throw and the config.edges!); the two as unknown as NodeContext fast-forward stubs that reach user code via ctx.runNode() on resume; maxConcurrency: 0 silently meaning unlimited; and the two bare eslint-disable require-yield.
CI is green on all three OS legs — I checked the individual job list, not the rollup. Everything here is mechanical; happy to re-review and approve as soon as the export suppression is gone.
For sequencing: Parts 1, 2 and 3 (#587, #588, #590) are approved, so this is not blocking anything upstream of it in the stack.
…ence Addresses the behavioral review feedback on PR #592 (Part 5 runner): - WorkflowConfig is now a discriminated union so exactly one of `edges` / `dynamicEntry` is required at compile time (the runtime throws stay for JS callers), retiring the `config.edges!` non-null assertion. - Introduce a real `NodeResult` type and widen `ctx.runNode()` / `ScheduleDynamicNode.schedule` to `NodeContext | NodeResult`, replacing the two `... as unknown as NodeContext` casts on the resume fast-forward paths that hid a "result is not a live context" bug from the compiler. - Cooperative sibling cancellation: a Workflow now owns an AbortController (chained to the invocation signal) threaded to each child via executeChildNode and aborted in cleanupPending, so an in-flight sibling stops when a node fails. The node runner exposes `ctx.abortSignal` on the non-timeout path too; an external abort lets a cooperative node wind down (no hard throw), while a `timeout` deadline still raises NodeTimeoutError. - WorkflowAgent drains inside try/finally, closing the channel and awaiting the producer if the consumer stops early (break or Runner cancel). - Restrict the plain-text HITL resume to the single-pending-interrupt case; a reply is no longer broadcast to every pause. - maxConcurrency must be a positive integer (0 no longer silently means "unlimited"; `undefined` is the sentinel), validated at construction. - Persist arbitrary node payloads verbatim across snake/camel round-trips: add `route` to both event PRESERVE_KEYS lists (output/agentState already present) and guard the agentState read with an isRecord narrowing instead of a cast. - Mark Workflow and WorkflowAgent @experimental; name the require-yield eslint-disable directives.
Addresses the public-surface review feedback on PR #592: - Rename the subclassing base class `Node` -> `WorkflowNode` (the `node()` factory is unchanged). `Node` shadowed the DOM / @types/node global in the flat @google/adk namespace and read confusingly next to `node()`; `WorkflowNode` lines up with `WorkflowAgent` / `WorkflowConfig`. Mark it @experimental. - Replace `export * from './workflow/index.js'` in index.ts with explicit named re-exports in common.ts, so the top-level surface is intentional (collisions become compile errors) and the workflow API also reaches the web entry point (index_web.ts re-exports common.ts). - Export the types reachable from the public NodeContext surface — AsyncQueue (via common.ts), NodeContextOptions / NodeResult / ScheduleDynamicNode / ScheduleDynamicNodeOptions (via the workflow barrel) — and drop the typedoc.json `intentionallyNotExported` block that was hiding, rather than fixing, that gap. `docs:check` stays clean.
Addresses the test-hygiene review feedback on PR #592: - Lift the duplicated `createIc()` / `driveWorkflow()` fixtures out of five test files (workflow, parallel, hitl_flow, dynamic_workflow, auth_gate) into the shared `test_helpers.ts` harness, removing ~200 lines and every `as unknown as Session` / `as unknown as BaseAgent` double cast (the shared helpers build a real Session via createSession and a real BaseAgent subclass). - Add coverage for the behavioral fixes: single-vs-multi pending plain-text resume (workflow_agent_test.ts), cooperative sibling cancellation on failure, maxConcurrency validation, and a snake/camel round-trip that verifies user-defined keys in output/route/agentState survive persistence. - node_execution_test uses executeChildNode where it needs the concrete child NodeContext (runNode's return type now widens to NodeContext | NodeResult).
AmaadMartin
left a comment
There was a problem hiding this comment.
Re-reviewed at d95bc230. The blocker is gone and it was fixed at the root rather than around it.
The intentionallyNotExported suppression is deleted, and AsyncQueue, ScheduleDynamicNode and NodeContextOptions are now genuinely exported from core/src/common.ts — reachable publicly via index.ts's export * from './common.js', and via index_web.ts too, which also closes the separate gap where workflow was missing from the web entry point. That was the one thing I held on last round: the suppression silenced the check that detects the defect instead of fixing the defect, on the very PR where the public barrel lands. Exporting the types and dropping the list is the right resolution.
The rest of the round is addressed, and several were fixed more thoroughly than I asked:
maxConcurrency: 0no longer silently means unlimited — it is validated as a positive integer and throws with the workflow name in the message. The trap is gone rather than documented.- The plain-text resume broadcast is fixed correctly.
resumeInputsFromPlainTextnow bails unless exactly one interrupt is pending, and the doc states the exact failure mode — a reply would otherwise reach every pause and at least one node would resume with data the user never gave it. That is the reasoning, not just the guard. config.edges!and bothas unknown as NodeContextfast-forward stubs are gone, so user code reached throughctx.runNode()on resume no longer receives a double-cast four-field stub.PRESERVE_KEYSnow carriesoutput,routeandactions.agentStatein both the camel and snake lists, so a resumed node's input survives a persistent session service intact.@experimentalis now on the public entry points, which is what lets Parts 6-8 keep moving without every rough edge becoming a compatibility commitment.
On the two remaining eslint-disable require-yield: I checked both and they are fine as written. Each now carries a specific reason rather than a generic one — the orchestration generator streams child events via ctx.channel/ctx.runNode and yields nothing itself, and runLiveImpl must be an AsyncGenerator to satisfy BaseAgent but only throws because live mode is unsupported. Those are real constraints, not silenced lint. Added suppressions dropped from 13 to effectively one cast (credentialResponse as unknown as Record<...>), which is not worth blocking on.
CI is green across ubuntu/macOS/Windows — verified against the individual job list, not the rollup. LGTM.
With this, Parts 1-5 (#587, #588, #590, #591, #592) are all approved and contiguous in merge order, so the stack can start landing from the bottom.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
Problem: With the engine core and node types in place, the workflow needs a runnable orchestrator and a public API.
Solution — Part 5 of 8 (dynamic scheduling + rehydration were merged into the runner PR because they are runner-coupled and share its tests). Stacked on Part 4.
Included:
workflow.ts— theWorkfloworchestrator: triggers, routing/fan-out, dynamic entry, resume/fast-forward.workflow_agent.ts—BaseAgentadapter so aWorkflowruns under the ADKRunner(streams via the sharedAsyncQueue).dynamic_node_scheduler.ts+utils/rehydration_utils.ts— dynamic scheduling and event-driven state reconstruction for resume.node.ts— thenode()user API.workflow/index.ts+core/src/index.ts— public barrel;typedoc.jsonmarks internalAsyncQueue/ScheduleDynamicNode/NodeContextOptionsas intentionally-not-exported.register_builtin_nodes.ts— side-effect module imported bynode()/workflowso the built-in Function/Tool/Parallel builders register even when those entry points are imported directly (not via the barrel).Adapted to Part 1's review APIs (
AsyncQueue,commonPrefixOf).Testing Plan
Tests (53):
workflow,workflow_advanced,routing,parallel,dynamic_workflow,dynamic_resume,resume,runner_integration,auth_gate,hitl. Full core suite 2444 green; docs:check clean; tsc clean.Manual E2E: N/A (covered by the runner integration tests; LLM-agent-as-node tests land in Part 6).
Checklist
Additional context
Stacked split — merge in order (…Part 4 → Part 5 → Part 6 → …).