feat(streaming): stream tool call argument deltas in TemporalStreamingModel#355
feat(streaming): stream tool call argument deltas in TemporalStreamingModel#355vkalmathscale wants to merge 2 commits into
Conversation
…gModel Wire ResponseFunctionCallArgumentsDeltaEvent into the streaming layer introduced in #333, so write-heavy tools (write_file, apply_patch) no longer freeze the UI for the duration of argument generation. The model now opens a per-function-call streaming context with a ToolRequestContent placeholder, emits ToolRequestDelta updates for each argument delta, and finalizes with a StreamTaskMessageFull containing the parsed arguments on ResponseOutputItemDoneEvent. Coalescing and mode dispatch are inherited from the existing streaming infrastructure -- no new flags or surface area. ModelResponse output is unchanged; activity determinism is unaffected. End-of-loop cleanup defensively closes any function-call contexts that didn't see a Done event (truncated stream or mid-stream exception). Adds two tests covering the happy path (well-formed JSON args -> deltas + parsed Full) and the malformed-args fallback (invalid JSON -> empty dict + WARNING log).
|
This PR is targeting The
See |
| try: | ||
| await call_context.close() | ||
| except Exception as e: | ||
| logger.warning(f"Failed to close tool request context: {e}") | ||
|
|
||
| elif isinstance(event, ResponseReasoningSummaryPartAddedEvent): |
There was a problem hiding this comment.
Double-close of streaming context on every successful function call — after
ResponseOutputItemDoneEvent closes the context (line 894), call_data['context'] is left non-None. The orphan-cleanup loop (lines 933–940) then iterates function_calls_in_progress.values() and closes every entry whose 'context' is not None — which includes every context that was already closed normally. close() will be called twice for every function call that finished cleanly, potentially publishing a duplicate "stream ended" event to Redis for each tool call in the response.
| try: | |
| await call_context.close() | |
| except Exception as e: | |
| logger.warning(f"Failed to close tool request context: {e}") | |
| elif isinstance(event, ResponseReasoningSummaryPartAddedEvent): | |
| try: | |
| await call_context.close() | |
| except Exception as e: | |
| logger.warning(f"Failed to close tool request context: {e}") | |
| finally: | |
| call_data['context'] = None | |
| elif isinstance(event, ResponseReasoningSummaryPartAddedEvent): |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agentex/lib/core/temporal/plugins/openai_agents/models/temporal_streaming_model.py
Line: 893-898
Comment:
**Double-close of streaming context on every successful function call** — after `ResponseOutputItemDoneEvent` closes the context (line 894), `call_data['context']` is left non-`None`. The orphan-cleanup loop (lines 933–940) then iterates `function_calls_in_progress.values()` and closes every entry whose `'context'` is not `None` — which includes every context that was already closed normally. `close()` will be called twice for every function call that finished cleanly, potentially publishing a duplicate "stream ended" event to Redis for each tool call in the response.
```suggestion
try:
await call_context.close()
except Exception as e:
logger.warning(f"Failed to close tool request context: {e}")
finally:
call_data['context'] = None
elif isinstance(event, ResponseReasoningSummaryPartAddedEvent):
```
How can I resolve this? If you propose a fix, please make it concise.Logging raw_args[:200] could leak partial file contents, PII, or secrets from write_file / apply_patch arguments into production log pipelines. Switch to logging only bounded metadata (tool name + raw arg byte count). The existing malformed-args test still passes since it asserts on the "Failed to parse tool call arguments" prefix, which is preserved.
Summary
TemporalStreamingModelalready streams text deltas and reasoning summary deltas to Redis viaStreamingTaskMessageContext, butResponseFunctionCallArgumentsDeltaEventwas being silently buffered intofunction_calls_in_progress[...]['arguments']with no per-delta publish. Consumers only saw the completed tool call surface later (after the activity returned, via downstream hooks if any).For write-heavy tools —
write_file,apply_patch, anything that puts a 2–20KB string into a single argument — the model spends multiple seconds generating the argument body, and the UI sees nothing until the entire activity finishes. The result is a frozen UI followed by an abrupt jump when the activity returns.This PR threads tool-call argument deltas through the same streaming machinery used for text and reasoning, riding on the
CoalescingBuffer+StreamingModeinfrastructure added in #333. The buffer's merge helpers already key ontool_call_idforToolRequestDelta, so coalescing, mode dispatch, and opt-out are inherited from existing infra.Design
TemporalStreamingModelnow opens astreaming_task_message_contextper function call (keyed off the call'soutput_index), withinitial_content=ToolRequestContent(...)and the model's configuredstreaming_mode. Three event handlers participate:ResponseOutputItemAddedEvent(type=function_call)function_calls_in_progress[output_index]['context'].ResponseFunctionCallArgumentsDeltaEventStreamTaskMessageDelta(delta=ToolRequestDelta(arguments_delta=..., tool_call_id=..., name=...))into the per-call context. The coalescing buffer merges consecutive deltas with the sametool_call_id.ResponseOutputItemDoneEvent(type=function_call)JSONDecodeError), emit a finalStreamTaskMessageFull(content=ToolRequestContent(...)), and close the context.End-of-loop cleanup defensively closes any function-call contexts that didn't see a
Doneevent (truncated stream or mid-stream exception).ModelResponseoutput is unchanged:output_itemsstill receives the same completeResponseFunctionToolCall. Activity determinism is unaffected — streaming is a side effect.What this does NOT change
StreamingModeis already the on/off knob. No new flag.streaming_mode="off"suppresses tool-arg deltas the same way it suppresses text deltas."per_token"publishes immediately;"coalesced"(default) batches at 50ms / 128 chars.TemporalStreamingHooks.on_tool_startis unchanged. It still fires after the activity returns and still emits aToolRequestContentFull message via thestream_lifecycle_contentactivity. See Caveats.Caveats
Overlap with
TemporalStreamingHooks.on_tool_start. Users who passTemporalStreamingHookstoRunner.runwill now see two persistedtask_messages per tool call: one created by the model (delta stream + final Full) and one created by the hook (Full only). Both land on the same Redis topictask:{task_id}with differentparent_task_message.ids, so a default UI will render two cards for the same logical tool call.This needs a follow-up to decide which path owns the canonical
ToolRequestemission. Options for review discussion:on_tool_start's Full emit when the model is also emitting (auto-detect via a workflow-instance flag, mirroring how_task_id/_trace_idare threaded today).on_tool_start's Full emit entirely in a follow-up major bump (the model becomes the single source of truth forToolRequestevents).Until that follow-up, users who want streamed tool args without duplicate emits should subclass
TemporalStreamingHooksand overrideon_tool_startto a no-op.Coalescing windows still apply. With the default 50ms / 128-char window, tool args render in ~50ms-granularity chunks rather than per-token. This is the same tradeoff already made for text streaming in perf(streaming): coalesce per-token publishes to Redis (50ms / 128-char window) #333, and the right default for write-heavy tools (UX value is "watch the artifact appear", not "see each token").
Malformed argument JSON. If the model produces invalid JSON for the args (truncated stream, hallucinated structure), the path logs a WARNING and emits
arguments={}in the finalToolRequestContent. The raw delta stream is preserved on the consumer side regardless — only the structured final view falls back.Test plan
test_streaming_model.py::TestStreamingModelFunctionCallArgsStreaming:ToolRequestContent, oneStreamTaskMessageDelta(ToolRequestDelta)perArgumentsDeltaevent preserving the delta text, and one finalStreamTaskMessageFull(ToolRequestContent)with parsed args.arguments={}in the final Full and logs a WARNING.test_streaming_model.pysuite passes (42/42).ruff checkclean on both modified files.streaming_mode="off"suppresses tool-arg deltas (only the final persisted message exists on close).cc reviewers familiar with #333's
CoalescingBufferdesign.Greptile Summary
ResponseFunctionCallArgumentsDeltaEventthrough the existingCoalescingBuffer+StreamingModeinfrastructure by opening astreaming_task_message_contextper function call (keyed onoutput_index), emittingToolRequestDeltaupdates per delta, and a finalStreamTaskMessageFullwith parsed JSON onResponseOutputItemDoneEvent. Text and reasoning paths are unchanged.TemporalStreamingHookswith this change will see twoToolRequesttask messages per tool call (one from the model stream, one fromon_tool_start) until a follow-up removes the hook's duplicate emit.call_data['context']is not set toNoneafterResponseOutputItemDoneEventcallsclose()— this was flagged in a prior review thread.Confidence Score: 3/5
Not safe to merge without addressing the double-close of streaming contexts on every successful function call (flagged in a prior review thread but unresolved in this revision).
A pre-existing P1 (double-close of already-closed streaming contexts in the orphan-cleanup loop) remains unaddressed —
call_data['context']is never nulled out after theResponseOutputItemDoneEventhandler closes it, so every successfully-completed function call context is closed twice, potentially publishing a duplicate stream-ended event per tool call. Score is pulled below the P1 ceiling of 4 because this affects all function calls in every response, not an isolated edge case.src/agentex/lib/core/temporal/plugins/openai_agents/models/temporal_streaming_model.py — specifically the
ResponseOutputItemDoneEventhandler (around line 894–897) wherecall_data['context']must be set toNoneafterclose()to prevent the orphan-cleanup loop from double-closing it.Important Files Changed
streaming_task_message_context; has a pre-existing double-close bug (flagged in previous review) where successfully-closed contexts are re-closed by the orphan-cleanup loop becausecall_data['context']is never set toNoneafter theResponseOutputItemDoneEventhandler callsclose().streaming_task_message_contextcalls sharing a singlereturn_valuemock, which is a fragile assumption when multiple context types coexist.Sequence Diagram
sequenceDiagram participant OAI as OpenAI Stream participant TSM as TemporalStreamingModel participant CTX as StreamingTaskMessageContext participant Redis as Redis (task:{task_id}) OAI->>TSM: "ResponseOutputItemAddedEvent(type=function_call)" TSM->>CTX: "__aenter__(initial_content=ToolRequestContent(args={}))" CTX-->>Redis: persist streaming task_message (IN_PROGRESS) loop Per argument token OAI->>TSM: "ResponseFunctionCallArgumentsDeltaEvent(delta=chunk)" TSM->>TSM: "call_data['arguments'] += chunk" TSM->>CTX: stream_update(StreamTaskMessageDelta(ToolRequestDelta)) CTX-->>Redis: "publish delta (CoalescingBuffer @ 50ms/128ch)" end OAI->>TSM: "ResponseFunctionCallArgumentsDoneEvent(arguments=full_str)" TSM->>TSM: "call_data['arguments'] = full_str (authoritative)" OAI->>TSM: "ResponseOutputItemDoneEvent(type=function_call)" TSM->>TSM: "json.loads(call_data['arguments']) => parsed_args" TSM->>CTX: "stream_update(StreamTaskMessageFull(ToolRequestContent(args=parsed_args)))" CTX-->>Redis: publish full message TSM->>CTX: close() Note over TSM,CTX: call_data['context'] NOT set to None here OAI->>TSM: ResponseCompletedEvent TSM->>TSM: "output_items = response.output" Note over TSM: Orphan-cleanup loop TSM->>CTX: close() again (double-close - call_data['context'] still non-None)Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix(streaming): drop raw tool args from ..." | Re-trigger Greptile