feat(workflow): built-in Function and Tool nodes (Part 3)

[kalenkevich]

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):

• Related: Support for Workflows #366

2. Or, if no issue exists, describe the change:

Problem:
Continuing the stacked split of the large feature/workflows branch. 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. Supports inputSchema/outputSchema validation and an auth gate for HITL (the gate's request processors land in Part 8).
• nodes/tool_node.ts — wraps a BaseTool as a node.

Both node modules self-register with the engine's node-builder registry (registerNodeBuilder) at import time, so buildNode() / isNodeLike() — and thus node() 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:

• I have added or updated unit tests for my change.
• All unit tests pass locally.

Bundled tests (9): workflow/schema_validation_test.ts (input/output schema coercion + rejection through driveNode) and workflow/node_builders_test.ts (registry wiring: function → FunctionNode with name resolution, unnamed-function error, tool → ToolNode, existing-BaseNode passthrough, and isNodeLike).

$ npx vitest run --project unit:core \
    core/test/workflow/schema_validation_test.ts \
    core/test/workflow/node_builders_test.ts
 Test Files  2 passed (2)
      Tests  9 passed (9)

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

• I have read the CONTRIBUTING.md document.
• I have performed a self-review of my own code.
• I have commented my code, particularly in hard-to-understand areas.
• I have added tests that prove my fix is effective or that my feature works.
• New and existing unit tests pass locally with my changes.
• I have manually tested my changes end-to-end.
• Any dependent changes have been merged and published in downstream modules.

Additional context

Stacked split — merge in order (…Part 2 → Part 3 → Part 4 → …). Diff: 4 files, +462.

[AmaadMartin]

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.

[AmaadMartin]

Not a nit. ToolNode calls the tool directly, bypassing the entire tool-execution chain that every other tool call in this repo goes through.

const response = await this.tool.runAsync({args, toolContext});

The canonical path is handleFunctionCallList in core/src/agents/functions.ts: plugin runBeforeToolCallback (step 1, line ~345), the agent's beforeToolCallbacks (step 2, line ~355), runOnToolErrorCallback when the tool throws (line ~379), plugin runAfterToolCallback (step 4, line ~406), the agent's afterToolCallbacks (step 5, line ~417), plus traceToolCall telemetry and the toolConfirmation lookup (lines ~324-333). None of that runs here.

Concretely: a plugin that audits, redacts or blocks tool calls silently stops working the moment the same tool is invoked from a workflow, and a tool that throws never reaches runOnToolErrorCallback. A sibling PR (#334) was flagged for exactly this. Either route through the shared helper, or state explicitly in the class doc that workflow tool nodes are outside the callback/plugin contract — but the first is what users will expect, since node(myTool) looks like "run my tool".

[AmaadMartin]

Not a nit. Only stateDelta is harvested off the tool's context; everything else the tool writes to toolContext.actions is silently discarded.

const stateDelta =
  Object.keys(toolContext.actions.stateDelta).length > 0
    ? {...toolContext.actions.stateDelta}
    : undefined;

The Context built on line 40 gets no eventActions, so it allocates a private one (core/src/agents/context.ts:56). What gets dropped:

• toolContext.saveArtifact() -> eventActions.artifactDelta (context.ts:112) — the artifact is written to the service but its version is never recorded on the session.
• toolContext.requestCredential() -> eventActions.requestedAuthConfigs (context.ts:123) — the auth request never reaches the runner, so the tool can never obtain credentials. Any auth-requiring tool deadlocks in a workflow.
• requestConfirmation() -> requestedToolConfirmations (context.ts:178) — the confirmation gate becomes a no-op.
• skipSummarization, escalate, transferToAgent.

Suggested fix — share the node's accumulator instead of hand-picking one field:

const toolContext = new Context({
  invocationContext: ctx.invocationContext,
  eventActions: ctx.actions,
  functionCallId: ...,
});

and then attach ctx.actions to the emitted event rather than only {stateDelta}. Caveat I did not fully verify: sharing ctx.actions means the node's own delta and the tool's delta merge into one object, so if you also adopt the "attach once" fix from my function_node.ts comment the two need to agree on who drains it.

[AmaadMartin]

Not a nit. A fresh random functionCallId per run means anything keyed on it can never be matched on the next turn.

functionCallId: randomUUID(),

Context.requestCredential and requestConfirmation key their entries by functionCallId (core/src/agents/context.ts:123, :178), and the resume flow looks those ids back up. Since this is regenerated on every run — including every retry and every resume — a resume response can never be matched to the request.

FunctionNode in this same PR gets this right and says why (function_node.ts:139-141):

// The credential key doubles as a deterministic interrupt id so the resume response matches across turns.

Derive it deterministically from what the engine already has, e.g. `${ctx.nodePath}:${ctx.runId}`, so the two node types agree on the rule.

[AmaadMartin]

Not a nit. Long-running tools are neither handled nor rejected — they silently look like completed calls.

if (response !== undefined && response !== null) {

BaseTool.isLongRunning exists (core/src/tools/base_tool.ts:68) and the canonical path keys off it: core/src/agents/functions.ts:441 treats a null response from a long-running tool as pending and puts the call id in longRunningToolIds so the invocation suspends. Here, a long-running tool that returns nothing yields no event at all, child.output stays undefined, and the graph advances as if the tool finished.

Note the machinery is right there: node_runner.ts:159 turns event.longRunningToolIds into child.interruptIds, which is exactly the suspend signal — ToolNode just never sets it. Either wire it up, or throw in the constructor when tool.isLongRunning so the failure is loud instead of a wrong result. Same question for streaming/generator tools, which runAsync's Promise<unknown> signature can't express anyway.

[AmaadMartin]

Nit. The emitted event carries output but no content, unlike every other node.

output: response,

FunctionNode.toEvent sets content: toContent(output) (function_node.ts:187) and so does the base implementation (base_node.ts:185). A ToolNode result is therefore invisible to anything that reads event.content — session history, UI rendering, and any downstream LLM node that rebuilds contents from history will not see that the tool ran or what it returned.

content: toContent(response),
output: response,

toContent is already exported from ../base_node.js, so this is one extra import. (There's also no functionResponse part, which is how a tool result normally appears in history — worth deciding deliberately rather than by omission.)

[AmaadMartin]

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.

[AmaadMartin]

Nit. The accumulated state delta is re-attached to every event the node yields, and it wins over deltas the handler set itself.

const stateDelta =
  Object.keys(ctx.actions.stateDelta).length > 0
    ? {...ctx.actions.stateDelta}
    : undefined;

ctx.actions.stateDelta is never drained, so a generator handler that yields N items emits the same growing delta N times — every event re-applies everything written before it. And on the isEvent branch:

Object.assign(event.actions.stateDelta, stateDelta);

the context delta overwrites keys the handler explicitly set on its own event. The precedence is backwards: the value the handler put on the event it is yielding should win.

event.actions.stateDelta = {...stateDelta, ...event.actions.stateDelta};

For the duplication, note you can't just clear ctx.actions.stateDelta — NodeContext builds its State over that same object (node_context.ts:95-98), so clearing it would make the node lose its own reads. Tracking which keys have already been attached is the safe version. I checked the read path but not every consumer of a repeated delta, so this may be benign in practice — worth a second look either way.

[AmaadMartin]

Nit. Spread order lets an explicitly-undefined name clobber the computed default.

super({name: config.name ?? tool.name, ...config});

If config has an own name key whose value is undefined, ...config overwrites the fallback and BaseNode throws 'Node name must be a non-empty string.'. That is reachable: buildNode forwards its BuildNodeOptions object verbatim (workflow_graph_utils.ts:157) and BuildNodeOptions.name is optional, so any caller building {name: opts.name, ...} — which is what the node() API in Part 6 will most naturally do — hits it. The tests here only ever call buildNode(new TestTool()) with no options, so nothing catches it.

super({...config, name: config.name ?? tool.name});

Same shape at function_node.ts:85 (super({name, ...config})); the builder there passes the same BuildNodeOptions object, and FunctionNodeConfig omitting name only stops the compiler, not the runtime spread. Worth fixing both.

[AmaadMartin]

Nit, optional. Two small things in this tail block.

The registerNodeBuilder(...) side effect sits between two helper functions — isAsyncIterable above it, isSyncGenerator below — so a reader has to scroll past a top-level side effect to find the last helper. tool_node.ts puts the registration last, which is the right shape; matching it here reads like a rebase artifact was cleaned up.

And in the guard:

typeof value !== 'string' &&
typeof (value as Iterable<unknown>)[Symbol.iterator] === 'function' &&
typeof (value as Generator<unknown>).next === 'function'

the string check is dead — a string has Symbol.iterator but no .next, so the third clause already excludes it. Dropping it leaves the same behaviour in two lines.

[AmaadMartin]

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.