feat(workflow): built-in Function and Tool nodes (Part 3) - #590
feat(workflow): built-in Function and Tool nodes (Part 3)#590kalenkevich wants to merge 4 commits into
Conversation
328c5d3 to
390b268
Compare
390b268 to
efa6d2b
Compare
kalenkevich
changed the title
AmaadMartin
left a comment
AmaadMartin
left a comment
There was a problem hiding this comment.
Reviewed the Part 3 delta only (both node modules plus the two new test files), verified against the head SHA and the surrounding engine from Parts 1-2. The FunctionNode side is largely sound — the auth gate's deterministic interrupt id and the Event/Content/null coercion in toEvent are right, and I confirmed error propagation is consistent between the two node types (neither swallows a throw, both let the runner's retry/failure path see it). ToolNode is where the substance is: it invokes tool.runAsync directly, so the plugin and before/after tool callback chain, the onToolError hook, the confirmation gate and long-running handling from core/src/agents/functions.ts are all bypassed, and everything the tool writes to its context except stateDelta is dropped. It also has no execution test. One thing I checked and can rule out: Event.output is in PRESERVE_KEYS (event.ts:371/:394), so structured node output survives the snake/camel round-trip.
| function coerceToolArgs(input: unknown): Record<string, unknown> { | ||
| let args: unknown = input; | ||
|
|
||
| if (isContent(args)) { |
There was a problem hiding this comment.
Not a nit. inputSchema does not apply on the path that most needs it, and the coerced args reach the tool unvalidated.
if (isContent(args)) {
args = extractText(args);
}BaseNode.validateInput deliberately skips genai Content (base_node.ts:151: if (!this.inputSchema || isContent(input)) return input;) on the assumption that nodes coerce it themselves. ToolNode does coerce it — into the tool's argument object — but never re-validates afterwards. So when the input is Content (i.e. produced by an LLM node, which is the whole point of Part 7), model-authored text is JSON.parsed and handed straight to tool.runAsync with inputSchema never applied and no check against the tool's own _getDeclaration() parameters. Model-populated fields are attacker-influenced; this is the boundary where that matters.
Minimum: run the coerced object back through this.validateInput/the tool declaration before calling runAsync.
Two smaller things in the same helper: JSON.parse('null') yields null, which falls through to return {} and invokes the tool with no arguments at all rather than reporting bad input; and the TypeError thrown below is a permanent input error that a user-configured retryConfig will happily retry (shouldRetryNode matches on error.name). Also 'must be a dictionary of tool arguments' reads as Python — "object" is the TS word.
| it('builds a ToolNode from a BaseTool', () => { | ||
| const node = buildNode(new TestTool()); | ||
| expect(node).toBeInstanceOf(ToolNode); | ||
| expect(node.name).toBe('test_tool'); | ||
| }); |
There was a problem hiding this comment.
Not a nit. ToolNode has no execution test at all — 117 lines of runtime logic with only this construction check.
it('builds a ToolNode from a BaseTool', () => {
const node = buildNode(new TestTool());
expect(node).toBeInstanceOf(ToolNode);
expect(node.name).toBe('test_tool');
});Nothing drives ToolNode.runImpl. Untested: that the tool is actually invoked with the coerced args, that a returned value lands on event.output, that toolContext state writes propagate, and every branch of coerceToolArgs (Content -> text, JSON string, empty string -> {}, array/scalar -> TypeError). driveNode already exists in test_helpers.ts and schema_validation_test.ts uses it for FunctionNode, so the harness is right there.
Several of the issues I flagged in tool_node.ts (dropped artifactDelta/requestedAuthConfigs, missing content, long-running tools completing silently) would each be caught by one such test.
c348697 to
f082675
Compare
cbf1f8a to
f58ca07
Compare
65a5160 to
b105f25
Compare
AmaadMartin
left a comment
AmaadMartin
left a comment
There was a problem hiding this comment.
Re-reviewed at b105f256. The blocking finding is properly fixed, not worked around.
ToolNode now executes through handleFunctionCallList, so the plugin before/after/onError callbacks, tracing, and the confirmation gate all run on a workflow tool call instead of being skipped by a direct tool.runAsync. That also fixes it at the source for #594 — requireConfirmation was inert inside workflows purely because this path bypassed the gate, so that whole class of problem goes away here rather than needing a patch in Part 7. Routing through the canonical path also means the tool's full actions (artifactDelta, requestedAuthConfigs, requestedToolConfirmations, escalate) now flow through instead of only stateDelta being hand-copied.
The other substantive items are addressed too, and in each case the comment explains the reasoning rather than just changing the line:
functionCall.idis now${ctx.nodePath}:${ctx.runId}instead of a freshrandomUUID()per invocation, so a credential or confirmation request can actually be matched to its resume response across turns and retries.this.validateInput(coerceToolArgs(input))re-validates after coercion, which closes the trust-boundary gap — model-authored text parsed out ofContentis now schema-checked before it reaches the tool, and the comment says explicitly that this is the only point where that happens.- The node now yields the canonical response event (keeping
functionResponsecontent for history) and setsoutputfrom it, instead of emitting output with no content.
Passing beforeToolCallbacks: [] / afterToolCallbacks: [] is the right call and I checked it deliberately: those are the agent-level lists, which a workflow node has no equivalent of, while plugin callbacks still fire via invocationContext.pluginManager. The comment already says so.
CI is green across ubuntu/macOS/Windows — verified against the individual job list rather than the rollup. LGTM.
b105f25 to
5f89957
Compare
5f89957 to
e4774ff
Compare
e4774ff to
05509b2
Compare
Part 3/9 of the feature/workflows split, stacked on the engine core. - nodes/function_node: wraps a plain function / async function / (async) generator as a node; supports input/output schema validation and an auth gate for HITL (the gate's processors land in Part 8). - nodes/tool_node: wraps a BaseTool as a node. Both self-register with the engine's node-builder registry (registerNodeBuilder) at import time, so buildNode()/isNodeLike() — and thus node()/graph parsing — turn a bare function or tool into the right node without the engine statically importing these modules. This is the registration side that Part 2's decoupling refactor was built for. Tests (9): schema_validation (input/output schema coercion + rejection) and node_builders (registry wiring: function -> FunctionNode with name resolution, unnamed-function error, tool -> ToolNode, existing-BaseNode passthrough, and isNodeLike). Full core suite green (2375 tests). The node() user API and the auth-gate integration test land in later parts (they need the runner/barrel).
… chain
Conformance with the updated Part 2 (restores a green build):
- registerNodeBuilder now passes the required `id` ('function' / 'tool').
- processAuthResume is called with a single params object.
ToolNode — route through the canonical execution path
(agents/functions.ts::handleFunctionCallList) instead of calling
tool.runAsync directly. This restores the plugin before/after/onError tool
callbacks, the confirmation gate, telemetry, and full `actions`
propagation (stateDelta, artifactDelta, requested credentials /
confirmations), and emits a canonical `functionResponse` event with
`content` — fixing review comments on the bypassed contract, dropped
context actions, and the missing content.
- Deterministic function-call id (`${nodePath}:${runId}`) so credential /
confirmation resume can match across turns (was a fresh UUID per run).
- Throw for a long-running tool at construction (suspend machinery lands
later) rather than silently completing the call.
- Re-validate the coerced args against `inputSchema` before invoking, so
model-authored (Content-path) args are checked; reword the args error
("object", not "dictionary").
- Spread `config` before the name fallback so an explicit `undefined`
name can't clobber it (same fix in FunctionNode).
FunctionNode:
- Attach each written state key to an event only once per run (no more
re-emitting the growing delta on every generator item), and let a
handler's own event delta win over the context delta.
- Move the builder registration to the end of the module and drop the
dead string check in isSyncGenerator.
Tests: add ToolNode execution coverage (invocation + args coercion
branches, state propagation, plugin-chain override, long-running guard)
and FunctionNode coverage (generator/Content/null/Event results, state
de-dup + precedence, auth-gate interrupt + resume).
…list Follow the registry removal from the engine core: the function and tool builders now live in node_builders.ts (NODE_BUILDERS) instead of calling registerNodeBuilder at module load. function_node.ts / tool_node.ts export only their node classes; no import-time side effects.
Rename the module-level builder constants functionBuilder/toolBuilder to FUNCTION_BUILDER/TOOL_BUILDER, matching the const naming used for NODE_BUILDERS / PARALLEL_WORKER_FACTORY.
05509b2 to
fad80df
Compare
2 participants
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 stacked split of the large
feature/workflowsbranch. With the engine core and its node-builder registry in place (Part 2), the workflow engine needs its first concrete node types.Solution:
This is Part 3 of 9 — the built-in Function and Tool nodes — stacked on Part 2.
Stacked on: #part2_pr_number (Part 2 — engine core). Please merge Part 2 first.
Included:
nodes/function_node.ts— wraps a plain function / async function / (sync or async) generator as a node. SupportsinputSchema/outputSchemavalidation and an auth gate for HITL (the gate's request processors land in Part 8).nodes/tool_node.ts— wraps aBaseToolas a node.Both node modules self-register with the engine's node-builder registry (
registerNodeBuilder) at import time, sobuildNode()/isNodeLike()— and thusnode()and graph parsing — turn a bare function or tool into the right node without the engine statically importing these modules. This is the registration side that Part 2's decoupling refactor was built for; the public barrel (Part 6) imports the node modules so registration is guaranteed in real usage.Intentionally deferred: the
node()user API (node.ts) and the auth-gate integration test move to Part 6 (they need the runner/barrel). Parallelism (Part 4), dynamic scheduling (Part 5), LLM-as-node (Part 7), and HITL processors (Part 8) follow.Testing Plan
Unit Tests:
Bundled tests (9):
workflow/schema_validation_test.ts(input/output schema coercion + rejection throughdriveNode) andworkflow/node_builders_test.ts(registry wiring: function →FunctionNodewith name resolution, unnamed-function error, tool →ToolNode, existing-BaseNodepassthrough, andisNodeLike).Full core suite green (2375 tests). Typecheck clean:
npx tsc --noEmit -p core/tsconfig.json.Manual End-to-End (E2E) Tests:
N/A — node-level units; graph/runner E2E coverage lands in Part 6.
Checklist
Additional context
Stacked split — merge in order (…Part 2 → Part 3 → Part 4 → …). Diff: 4 files, +462.