Repository navigation
feat(streaming): native tool calls during SSE streaming - #971
vernonstinebaker wants to merge 2 commits into
Conversation
Decouple native tool-call support from the streaming path. Previously the
agent loop disabled native tools whenever a stream callback was attached,
forcing tools into a prompt-injection format that some models could not
reliably emit. The streaming protocol (StreamChunk / StreamChatResult) also
had no field to carry tool calls, so even if a provider parsed them they
were dropped at the agent boundary.
Changes:
- providers/root: add `tool_calls` to StreamChatResult and a
`supportsStreamingNativeTools()` vtable slot, defaulting false.
- providers/sse: add a ToolCallAccumulator that parses `delta.tool_calls`
(OpenAI-compatible) across SSE chunks and emits a complete tool_calls
slice on the final StreamChatResult.
- providers/{openai,compatible}: opt in to streaming native tools.
- providers/{router,reliable}: delegate the capability conservatively.
- providers/anthropic: explicitly report false (Anthropic streaming
tool-use uses a different event shape and is left for a follow-up).
- agent/root: gate native tools on `supportsStreamingNativeTools()` when
streaming (not on `!is_streaming`), and propagate
`stream_result.tool_calls` into the agent loop instead of dropping it.
Adds regression tests asserting native tools are requested during streaming
when the provider advertises the capability, and that tool_calls emitted on
the streaming path reach the agent loop.
Validation: zig build test --summary all → 7362/7371 passed (9 skipped),
0 failures, 0 leaks.
|
Marking as draft pending #970 (fix(cli): handle arrow keys in agent REPL). Both PRs modify overlapping regions of This PR is complete and validated on its own ( |
|
Merged into the |
…sted Approved intake U-1/U-2/U-4 complete and upstream authors notified with commit links and intake-correction feedback. Landed-in-fork notes added to our nine still-open upstream PRs (nullclaw#987, nullclaw#971, nullclaw#970, nullclaw#966, nullclaw#963, nullclaw#962, nullclaw#959, nullclaw#954, nullclaw#953). Upstream PRs cannot be marked merged (no write access; re-applied commits never trigger GitHub merge detection), so these comments are the provenance record.
Incremental PR-by-PR plan for exercising the new committer access: wave 0 pre-flight, wave 1 docs (ours first; nullclaw#776 needs three verified corrections, nullclaw#777 must not merge with its 0.15.2 pin), wave 2 our code PRs smallest-first, wave 3 nullclaw#987 then un-drafted nullclaw#971. Per-code-PR loop: merge, fork sync, suite, 4-target build, 4-host deploy, two-turn smoke. Docker/OrbStack recorded as wave-4 decision (upstream nullclaw#449 still open; images publish via nullbuilder). Docs-only merges flagged to skip rebuild (binary cannot change) — pending user confirmation.
DonPrus
left a comment
There was a problem hiding this comment.
Reviewed the agent integration, all changed provider implementations, existing streaming consumers, current main, and both discussion comments. The ownership transfer into the normal tool dispatcher is useful, but I am leaving this draft without approval because the new executable tool-call path still has these blockers:
-
P1 — Do not execute tool calls salvaged from an incomplete/failed stream (
src/providers/sse.zig:769–805,ToolCallAccumulator.toOwnedToolCalls). The existing partial-text recovery branches now return accumulated tool calls as well. After some text plus a tool name arrives, a curl timeout/nonzero exit can therefore produce an executable call even if its argument deltas never arrived; the accumulator substitutes{}for missing arguments. Successful EOF also does not require a completed tool-call choice. Please track completion for the selected choice and only expose complete calls; partial text recovery must not authorize partially received tool execution. Cover a disconnect between name and argument deltas and a nonzero curl exit after mixed text/tool output. -
P1 — Query streaming tool capability for the actual routed provider (
src/providers/router.zig:250–255,src/providers/reliable.zig:610–612). Router checksdefault_model, whilestreamChatImplresolves the requested model. Reliable checks onlyinner, but can select an explicit extra/vision provider. With an OpenAI default and an Anthropic request, the agent enables native schemas and omits text-mode parameter instructions even though the selected Anthropic streaming implementation intentionally cannot recover native calls. Please make this capability model/request-aware (as vision support already is), and test both directions of a mixed-provider route. -
P2 — Keep tool calls from different choices separate (
src/providers/sse.zig:97–140). Text uses the first choice, but the accumulator visits every choice and keys slots only by tool-call index. A two-choice response with index 0 in both merges argument strings into{"path":"a"}{"path":"b"}, potentially combining one choice's ID with another's function name. I reproduced this with an isolated regression test. Please select the same choice as text or explicitly key/select by choice index. -
P2 — Preserve ownership and propagate allocation errors (
src/providers/sse.zig:89,:132–133). Replacing a repeated function name frees the old allocation before the fallible duplicate; on OOM the slot still points at freed storage and deferred cleanup frees it again. Allocate the replacement before releasing the old value. JSON parsing also catches OOM as a harmless malformed event, silently dropping argument fragments; an injected allocation-failure test confirmed this. Please propagate OOM and add allocation-failure coverage across repeated-name and argument assembly paths.
Analogous-path follow-up: OpenRouterProvider.streamChatImpl already uses the same SSE implementation and accepts native schemas, but its vtable is not opted into the new capability. It would be good to cover it in the same feature once the completion/routing contracts above are fixed.
Validation: 417/419 targeted provider/streaming tests passed; the two additional probes for cross-choice corruption and swallowed OOM failed as described. Temporary probes were saved locally and removed from the checkout. Current main merges cleanly at review time. I also read the draft dependency note about #970; resolving that dependency does not by itself address the findings above. No live provider calls were made.
Addresses the review findings on nullclaw#971. P1 -- a truncated stream could produce an executable tool call. `toOwnedToolCalls` completed any slot that had a name, substituting `{}` when no argument deltas had arrived. A curl timeout or nonzero exit after a tool name but before its arguments therefore yielded a call the agent would run with invented input. Two changes: - `ToolCallSlot.saw_arguments` records that an `arguments` key actually arrived, even when its value was the empty string, so "nothing arrived" is no longer indistinguishable from "this call takes no arguments". A slot is complete only when it has a name and `saw_arguments`; the `{}` substitution is gone. - `finalizeStreamResultWithToolCalls` takes a `tool_calls_trusted` flag. The three recovery paths (child wait error, nonzero curl exit, abnormal termination) pass false, so salvaged text is still returned but no tool call is executed. Only the clean-exit path passes true. P1 -- capability was asked of the wrong provider. The router consulted `default_model` while `streamChatImpl` resolves the requested model, and the failover wrapper consulted only `inner` while `streamChatImpl` can select an explicit extra or vision provider. With an OpenAI default and an Anthropic request, native schemas were enabled against an implementation that intentionally cannot recover native calls. Added `supports_streaming_native_tools_for_model` to the provider vtable, mirroring the existing `supports_vision_for_model` precedent, and wired the two agent call sites to ask about `turn_model_name` -- the model the turn will actually use. The router resolves the requested model; the failover wrapper mirrors `streamChatImpl`'s own selection via `resolveProviderTarget`, since vision selection needs the request and is not available at this call site. P2 -- tool calls from different choices were merged. Text comes from choices[0] but the accumulator visited every choice while keying slots only by tool-call index, so a two-choice response with index 0 in both concatenated their argument strings into `{"path":"a"}{"path":"b"}`, pairing one choice's id with another's function name. The accumulator now reads the same choice as the text. P2 -- a failed allocation left a dangling slot. The repeated function name freed the old value before the fallible duplicate, so on OOM the slot pointed at released storage and the deferred cleanup freed it again. The replacement is now duplicated before the old value is released. Related: `appendFromJsonPayload` swallowed `error.OutOfMemory` from the JSON parser as a malformed event; that is now propagated, so an allocation failure surfaces instead of silently dropping argument fragments.
|
All four findings addressed in P1 — a truncated stream could execute a tool callYour reproduction was right, and it was two defects:
P1 — capability was asked of the wrong providerAdded
Test covers both directions: a capable default answers yes for itself and no for a P2 — cross-choice mergingThe accumulator now reads P2 — ownership and OOMThe repeated function name now duplicates before releasing the old value, so an OOM can't leave the slot pointing at freed storage for the deferred cleanup to free again. Also: One thing to know before you reviewThe branch is 115 commits behind main and, as it stood, was not green: Validation
Left alone
|
Summary
Decouple native tool-call support from the streaming path so that providers which support native tools during streaming can actually emit them.
Previously the agent loop disabled native tools whenever a stream callback was attached, forcing tools into a prompt-injection format that several models could not reliably emit. Worse, the streaming protocol (
StreamChunk/StreamChatResult) had no field to carry tool calls, so even when a provider parsed them from the SSE deltas they were dropped at the agent boundary.Background
The streaming implementation predates native tool support in the OpenAI-compatible protocol. When streaming was added, the response protocol was text-delta-only (
StreamChunk{ delta, is_final, token_count }), and the streaming call site passed.tools = null. The explicitnative_tools_enabled = !is_streaming and ...gate was added later (during a logging refactor) and codified a limitation that had already become structural rather than intentional — streaming tool-call deltas have been a documented part of the OpenAI-compatible protocol for years.This restores provider-protocol parity for the streaming path.
Changes
tool_callstoStreamChatResultand asupportsStreamingNativeTools()vtable slot, defaulting tofalse(conservative — providers must opt in).ToolCallAccumulatorthat parsesdelta.tool_calls(OpenAI-compatible format, including parallel calls indexed byindex) across SSE chunks and emits a completetool_callsslice on the finalStreamChatResult.false. Anthropic streaming tool-use uses a different event shape (input_json_deltacontent blocks) and is left as a follow-up to keep this PR focused.supportsStreamingNativeTools()when streaming (rather than on!is_streaming), and propagatestream_result.tool_callsinto the agent loop instead of dropping them.Tests
tool_callsemitted on the streaming path reach the agent loop (not dropped).ToolCallAccumulatorunit tests for delta accumulation, parallel-indexed calls, and malformed-input safety.Validation
0 failures, 0 leaks (
std.testing.allocator).zig fmt --check src/clean.Security / Risk
false): only OpenAI-compatible providers opt in. Adding the capability to a provider that does not actually parsedelta.tool_callswould silently drop tool calls, so the opt-in is intentional per provider.Scope
Provider streaming protocol only. No config schema, vtable signatures beyond the new
supportsStreamingNativeTools()slot, or user-facing setup changes. Anthropic streaming tool-use is explicitly out of scope for this PR.