feat(workflow): LLM-agent-as-node, task mode, and node-as-tool (Part 6) - #593
feat(workflow): LLM-agent-as-node, task mode, and node-as-tool (Part 6)#593kalenkevich wants to merge 6 commits into
Conversation
AmaadMartin
left a comment
AmaadMartin
left a comment
There was a problem hiding this comment.
Reviewed the three integration seams rather than line-by-line. The node lifecycle is handled correctly on the node-as-tool path (it goes through executeChildNode, so timeout/retry/input validation apply), and the transfer loop is capped — but the node->tool->node path itself has no depth or cycle guard, and task mode is documented as a loop while being implemented as a single pass with no failure path when finish_task is never called. Two instanceof checks and a set of unused InvocationContext fields are the other structural items. Comments are advisory; the recursion and task-termination ones are the two I would not merge without an answer to.
| for await (const event of agent.runAsync(agentIc)) { | ||
| const finishCall = getFunctionCalls(event).find( | ||
| (fc) => fc.name === FINISH_TASK_TOOL_NAME, | ||
| ); | ||
| if (finishCall) { | ||
| // Remember the latest finish_task args; wait for the success function | ||
| // response before terminating (a validation error lets the LLM retry). | ||
| pendingArgs = {...(finishCall.args ?? {})}; | ||
| yield event; | ||
| continue; | ||
| } | ||
|
|
||
| if (pendingArgs !== undefined && isFinishTaskSuccessResponse(event)) { | ||
| const output = finishTool.extractOutput(pendingArgs); | ||
| event.output = output; | ||
| event.nodeInfo = {...(event.nodeInfo ?? {}), messageAsOutput: true}; | ||
| if (agent.outputKey && output !== undefined) { | ||
| ctx.actions.stateDelta[agent.outputKey] = output; | ||
| } | ||
| yield event; | ||
| return; | ||
| } | ||
|
|
||
| yield event; | ||
| } | ||
| } |
There was a problem hiding this comment.
Not a nit. Task mode does not loop, and it has no failure path when the agent never calls finish_task.
for await (const event of agent.runAsync(agentIc)) {The docs promise a loop — llm_agent.ts:271 says task mode "runs a multi-round loop until it calls finish_task", and the docstring above says "the agent loops (LLM <-> tools) until it calls the finish_task tool" — but this is a single pass over agent.runAsync with no re-prompt. LlmAgent.runAsyncImpl breaks out on the first final response (llm_agent.ts:741-746), so a model that answers in plain text without calling finish_task ends the pass, drops out of this for await, and the generator returns normally.
On that path nothing sets the output: unlike runWithTransfers, runTaskMode never calls maybeSetOutput, so child.output stays undefined, the node is reported COMPLETE, and downstream {Class.field} / <field from node> placeholders silently resolve to nothing. A task that fails to finish should be a FAILED node or a re-prompt, not an empty success. Either throw when the loop exits with pendingArgs === undefined, or add the real loop with an explicit cap.
Related, and the reason the cap matters: nothing bounds this at all. runAsyncImpl is while (true), and FinishTaskTool.processLlmRequest injects "Do NOT call finish_task prematurely. Use your available tools to fully complete every aspect of the task first" — an instruction that deliberately pushes the model to keep calling tools. Task mode is therefore the one place in the workflow engine with an unbounded LLM+tool spend and no maxSteps / iteration cap. A named constant next to MAX_TRANSFER_DEPTH would match how transfers are already handled here.
910f8af to
16b2173
Compare
215f667 to
92464c5
Compare
kalenkevich
changed the title
AmaadMartin
left a comment
AmaadMartin
left a comment
There was a problem hiding this comment.
Re-reviewed at head 92464c5b. All eight findings from the prior round are resolved, including the two I would not have merged without — the node-as-tool recursion and task-mode termination.
What I verified against the source at this head:
- Node→tool→node recursion (blocker) — fixed.
MAX_NODE_TOOL_DEPTH = 8is enforced innode_tool.ts:144againstnodeToolDepth, a newreadonlyfield onInvocationContext(invocation_context.ts:226) thatrunNodeincrements viaic.clone({nodeToolDepth: ic.nodeToolDepth + 1})(node_tool.ts:152), so the depth carries across the spawned agent runs.runIdnow requiresfunctionCallIdand throws when absent (node_tool.ts:136-142), andnodePath: ''(node_tool.ts:159) removes the doubled path segment. - Task-mode termination (blocker) — fixed.
runTaskModenow throws when the agent finishes without a successfulfinish_task(llm_agent_wrapper.ts:155-158) instead of silently reporting COMPLETE with no output; the turn loop is bounded by the invocation'smaxLlmCalls. - Paused-node event queue — fixed. The
?? new AsyncQueue()fallback is gone; a missingic.eventQueuenow throws (node_tool.ts:126-132), and the structural cast was removed. instanceoftype detection — fixed.llm_agent.ts:340usesisBaseNode(now atype-only import) andrequest_input_llm_request_processor.ts:61usesisNodeTool— both brand-based guards. Remaininginstanceofuses are onlye instanceof Errornarrowing.InvocationContext.clone()extracted (invocation_context.ts:292) with the by-value/by-reference caveat documented; theas unknown as InvocationContextParamsdouble cast is gone.agentStates/endOfAgents(previously dead) were removed.- Public exports — fixed at the cause.
WorkflowInstructionScopeandInvocationContextParamsare exported fromcommon.tsandindex.ts;FinishTaskTool,NodeTool,LLMAgentWrapper/Configare exported too.typedoc.jsonis untouched — nointentionallyNotExportedband-aid. - Suppression sweep — src is clean. The
as neveratnode_tool.ts:65is gone (inline narrowing). Noany/as any/as unknown as/@ts-expect-errorinsrc; the threerequire-yielddisables are throw-only/return-only generators (the documented exception, consistent with the existingStartNode). Diff hygiene is clean (no.only/.skip/console.log).
CI: run-tests passed on ubuntu-latest, macOS-latest and the generic job. The one Windows failure was code_executors/unsafe_local_code_executor_test.ts "should execute shell code and return stdout" timing out at 5000ms — a documented pre-existing Windows flake in a file this PR does not touch (the same test passed on ubuntu and macOS). I re-triggered the Windows job; merge should confirm it green.
Only residual, non-blocking: a few as unknown as/as never casts remain in test fixtures (the nit portion of the suppression sweep) — author's discretion. LGTM.
|
Status update on the CI caveat in my approval above — Windows is now green, on the same head ( It took three attempts to get there, and the failure rotated each time rather than repeating:
A different unrelated test failing on each attempt, always a timeout, always subprocess-adjacent — and this PR touches neither Worth flagging beyond this PR: #591 also needed three attempts on its validation run for the same reason. So the whole stack is paying roughly ten minutes per retry on an intermittent Windows failure that has nothing to do with the changes under review. That seems worth a separate issue — either raising the timeout on those two suites on Windows or marking them flaky — rather than everyone re-running past it. |
92464c5 to
591bc8e
Compare
591bc8e to
cd3eb3d
Compare
cd3eb3d to
084ef29
Compare
084ef29 to
864270a
Compare
Part 6/9 of the feature/workflows split. Lets agents participate in workflows.
- nodes/llm_agent_wrapper: runs a BaseAgent as a workflow node — streaming,
transfer_to_agent hand-offs, workflow instruction scope, and task mode (loops
until the agent calls finish_task, whose args become the node output). It
registers the agent node-builder, explicitly excluding BaseTool (which also
exposes runAsync) so tool-before-agent precedence holds regardless of
registration order.
- nodes/node_tool: exposes a node/workflow as a tool an agent can call.
- tools/finish_task_tool: the finish_task tool backing task mode.
- agents/llm_agent: task mode (mode/finishTaskTool) and registers the
request-input + request-confirmation LLM request processors.
- agents/processors/request_input_llm_request_processor: agent-side HITL
(request user input mid-run).
- agents/{invocation_context,instructions}, basic_llm_request_processor:
workflow instruction scope and {Class.field}/<field from node> placeholder
resolution.
Wired into the public barrel (LLMAgentWrapper, NodeTool) and
register_builtin_nodes; llm_agent 3-way merged onto current main.
Tests: node_api (16), llm_agent (7), multi_agent (3), instructions (37), plus
the workflow integration suite (15 files / 35 tests, recorded model responses).
Full core suite green (2476), integration workflows green, docs:check + tsc
clean.
Rebased Part 6 onto the current Part 5 and reconciled with the Parts 2-5 conventions: - executeChildNode now takes a single params object (node_tool.ts). - Move the LLM-agent-wrapper builder into the static node_builders.ts const list; drop its registerNodeBuilder self-registration and the now-obsolete register_builtin_nodes.ts (the const list replaces side-effect registration).
node-as-tool (node_tool.ts): - Bound node -> tool -> node recursion with a MAX_NODE_TOOL_DEPTH cap, tracked via a new immutable InvocationContext.nodeToolDepth carried through a depth+1 clone (so it survives agent-run ic clones). - Require the invocation event queue and a function-call id (throw otherwise) instead of falling back to a dead queue / a collapsing runId; drop the structural cast on eventQueue. - Pass an empty parent nodePath so the child path is a single segment, not the node name doubled. - Derive the tool parameter schema by narrowing isZodObject inline (drops the `as never`). instanceof -> brand guards: - llm_agent.ts uses isBaseNode; the request-input processor uses isNodeTool (new brand + guard on NodeTool). Both imports become type-only, removing the agents -> workflow value cycle. task mode (llm_agent_wrapper.ts): throw when a task-mode agent ends without a successful finish_task, instead of reporting the node COMPLETE with no output (its turn loop stays bounded by the invocation maxLlmCalls). InvocationContext: add clone(overrides) and use it for both the workflow-instruction-scope and node-tool-depth children (removes the `as unknown as InvocationContextParams` cast); remove the unused agentStates / endOfAgents fields; export WorkflowInstructionScope from common.ts / index.ts (public field type) instead of suppressing the TypeDoc warning.
Replace the duplicated hand-rolled createIc (with `as unknown as Session` / `as unknown as BaseAgent`) in the Part 6 workflow tests with the shared test_helpers.createIc (createSession + a real BaseAgent), removing the repeated double-casts the review flagged.
Part 5 replaced the `export * from './workflow/index.js'` star in index.ts with explicit named re-exports in common.ts, so Part 6's public additions must be listed there too: - Add LLMAgentWrapper / NodeTool and the LLMAgentWrapperConfig type to the common.ts workflow block (they reach the web entry point this way, and the block now mirrors the barrel exactly). - Update node_api_test to subclass the renamed `WorkflowNode` base class.
864270a to
cce9d2d
Compare
3 participants
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
Problem: Workflows need to run ADK agents as nodes (and expose nodes/workflows to agents as tools).
Solution — Part 6 of 8. Stacked on Part 5.
Included:
nodes/llm_agent_wrapper.ts— runs aBaseAgentas a node: streaming,transfer_to_agenthand-offs, workflow instruction scope, and task mode (loops until the agent callsfinish_task, whose args become the node output). Registers the agent node-builder, explicitly excludingBaseTool(which also exposesrunAsync) so tool-before-agent precedence holds regardless of registration order.nodes/node_tool.ts— exposes a node/workflow as a tool an agent can call.tools/finish_task_tool.ts— thefinish_tasktool backing task mode.agents/llm_agent.ts— task mode (mode/finishTaskTool) + registers the request-input & request-confirmation processors (3-way merged onto current main).agents/processors/request_input_llm_request_processor.ts— agent-side HITL (request user input mid-run).agents/{invocation_context,instructions},basic_llm_request_processor— workflow instruction scope and{Class.field}/<field from node>placeholder resolution.Wired into the barrel (
LLMAgentWrapper,NodeTool) andregister_builtin_nodes.Testing Plan
Unit tests:
node_api(16),llm_agent(7),multi_agent(3),instructions(37). Plus the workflow integration suite (15 files / 35 tests, recorded model responses). Full core suite 2476 green; integration workflows green; docs:check + tsc clean.Manual E2E: Covered by the integration suite (replayed model responses).
Checklist
Additional context
Stacked split — merge in order (…Part 5 → Part 6 → Part 7 → …).
llm_agent.tswas 3-way merged onto currentmain.