Analyzed GitHub PR for tool call argument handling
Public
Analyzed GitHub PR for tool call argument handling
Agent
Pi
Model
Claude Opus 4.5 (latest)
Started
Jan 24, 2:04 AM
Messages30
Downloads0
Reviewed full PR details including description, comments, commits, and changed files.
UnderstandIdentified and read linked issues referenced in the PR.
UnderstandAnalyzed code changes and their impact on tool call argument handling.
UnderstandClarified variable reset behavior and effect on streaming tool results.
Verify
You are given one or more GitHub PR URLs: https://github.com/badlogic/pi-mono/pull/917 this looks fishy
For each PR URL, do the following in order:
packages/*/CHANGELOG.md files. Report whether an entry exists. If missing, state that a changelog entry is required before merge and that you will add it if the user decides to merge. Follow the changelog format rules in AGENTS.md. Verify:
### Breaking Changes, ### Added, ### Fixed, etc.)Fixed foo ([#123](https://github.com/badlogic/pi-mono/pull/123) by [@user](https://github.com/user))### Breaking Changes, not just ### FixedOutput format per PR: PR: Changelog:
If no issues are found, say so under Bad and Ugly.
The user wants me to review a GitHub PR. Let me start by fetching the PR details.
>gh pr view 917 --repo badlogic/pi-mono --json title,body,comments,author,state,additions,deletions,changedFiles,baseRefName,headRefName,commits,labels>gh pr diff 917 --repo badlogic/pi-monoNow let me read the full source file to understand the context better. I need to read the openai-responses.ts file to see how this change fits into the overall code.
Let me read the full source file to understand the context:
Let me now analyze the PR thoroughly. I've read the file and the changelog.
Looking at the diff:
The PR adds handling for response.function_call_arguments.done events, which some providers (like LM Studio with GLM-4.7-Flash) send instead of streaming arguments via delta events.
When the response.output_item.done event comes for a function_call, it now checks if arguments were already parsed from the done event (stored in currentBlock.partialJson) before falling back to item.arguments.
The fix also adds currentBlock = null after handling a function_call completion, which wasn't there before.
Let me check:
Looking at the code more carefully:
Good things:
done event rather than streaming them via deltas.currentBlock.partialJson if available, falling back to item.arguments.Potential issues:
response.function_call_arguments.done handler, it sets currentBlock.arguments = parseStreamingJson(currentBlock.partialJson). But event.arguments should be complete JSON, not streaming/partial JSON. Should it use JSON.parse(event.arguments) directly instead?Actually, looking at this more carefully:
event.arguments in currentBlock.partialJsonparseStreamingJson which is for incomplete JSONdone events should have complete JSONThis seems like it would work, but it's slightly inconsistent. The parseStreamingJson function will work on complete JSON too, so it's not a bug per se.
The currentBlock = null assignment after function_call completion mirrors the pattern used for other item types like reasoning and message, so that's consistent. I should also verify whether tests exist for this scenario or if new tests need to be added to cover the done event handling.
Let me check for any related tests and look at the parseStreamingJson function to understand its behavior better:
>rg -l "function_call_arguments" packages/ai/test/ 2>/dev/null || echo "No test files found"The parseStreamingJson function first tries JSON.parse for complete JSON, then falls back to partial-json for incomplete JSON. So using it on complete JSON is fine.
Now let me check the README and docs to see if they need modifications:
Now let me check if documentation needs updates:
>rg -l "openai-responses|LM Studio|function_call_arguments" packages/coding-agent/README.md packages/coding-agent/docs/ 2>/dev/null | head -5The README mentions openai-responses. Let me check what it says - is it related to setting up LM Studio or similar providers?
>rg -B2 -A5 "LM Studio|lm.studio|lmstudio|openai-responses" packages/coding-agent/README.md packages/coding-agent/docs/*.md 2>/dev/nullThe docs mention LM Studio but this is about custom model configuration. The bug fix doesn't change any public API or require documentation updates - it's an internal fix for how the OpenAI Responses API handles certain providers' behavior.
Now let me check the current main branch to see what's different. Looking at the diff again, I need to identify any issues:
response.function_call_arguments.done - this seems correctcurrentBlock.partialJson if available when parsing arguments in response.output_item.done - this is the key fixcurrentBlock = null after function_call handling - this was missing before and could be a bugWait, I'm looking at this more closely. In the original code, there's no currentBlock = null after handling function_call in response.output_item.done. But for reasoning and message types, there is. This inconsistency was a bug - the PR fixes it.
Also, I notice that in the fix:
const args = currentBlock?.type === "toolCall" && currentBlock.partialJson ? JSON.parse(currentBlock.partialJson) : JSON.parse(item.arguments);This uses JSON.parse(currentBlock.partialJson) instead of parseStreamingJson. But earlier in the response.function_call_arguments.done handler, it uses parseStreamingJson. This inconsistency is intentional - at the done event, the JSON should be complete, so JSON.parse is appropriate.
Let me think about edge cases:
What if item.arguments is empty "{}"? This would be handled correctly.
What if currentBlock.partialJson is empty or malformed? The JSON.parse would throw. But parseStreamingJson was used earlier to set currentBlock.arguments, so this path shouldn't have malformed JSON.
There's a potential issue with how partialJson accumulates - if both delta and done events arrive, the done event should provide the complete JSON, but I need to verify the flow doesn't cause overwrites or data loss when transitioning from streaming to completion.
Looking at the PR description again, the core problem is that response.output_item.done sends empty {} arguments when the provider uses response.function_call_arguments.done instead of deltas. The fix handles this by falling back to currentBlock.partialJson, which captures the complete arguments from whichever event type the provider sends.
One concern: what if currentBlock is null when response.output_item.done arrives? The code uses optional chaining (currentBlock?.type === "toolCall"), so null values are handled safely. There's also a guard checking both currentItem?.type === "function_call" and currentBlock?.type === "toolCall" to ensure the block exists and is the right type before accessing its arguments. Since the arguments originate from the LLM provider, data integrity depends on the provider's behavior.
The core fix is sound—it addresses a real compatibility issue by preferring accumulated partial JSON from deltas when available, falling back to the full arguments string otherwise. However, there are some gaps: no changelog entry was added, the edge case lacks test coverage, and the external contributor process wasn't followed. For existing behavior, the change is backward compatible since providers sending deltas will use currentBlock.partialJson while others fall back to the original item.arguments path.
PR: https://github.com/badlogic/pi-mono/pull/917
Changelog:
packages/ai/CHANGELOG.md under [Unreleased]. Required before merge.Good:
response.function_call_arguments.done instead of streaming deltascurrentBlock.partialJson (populated from done event) over item.arguments (which is empty "{}" in this case)currentBlock = null after function_call handling in response.output_item.done, matching the pattern used for reasoning and message types (this was a pre-existing bug)parseStreamingJson function tries JSON.parse first, so using it on complete JSON from the done event is correctBad:
function_call_arguments.done without deltas)parseStreamingJson in the done handler but JSON.parse in the completion handler is inconsistent (both work, but mixing styles)Ugly:
Questions or Assumptions:
Change summary:
response.function_call_arguments.done event that stores complete arguments in currentBlock.partialJsonresponse.output_item.done handler for function_call to prefer pre-parsed arguments from currentBlock.partialJson over potentially empty item.argumentscurrentBlock = null cleanup after function_call completion (bug fix unrelated to the main issue)Tests:
function_call_arguments.done without prior delta events.Verdict: The code looks correct and the fix is sensible, but the contributor needs to:
The "fishy" part you noticed is likely the external contribution process bypass and lack of tests. The actual code change is sound.
are you sure the missing currentBlock = null is a preexisting bug? also, what effect does this have on responses from providers that DO support tool result streaming?
Let me re-examine the code more carefully. I need to look at what happens with currentBlock = null and whether its absence was actually a bug.
Looking at the original code flow:
response.output_item.added with function_call type:
currentItem = itemcurrentBlock = { type: "toolCall", ... }output.contenttoolcall_startresponse.function_call_arguments.delta:
currentBlock.partialJson += event.deltaparseStreamingJsontoolcall_deltaresponse.output_item.done with function_call type:
toolCall objecttoolcall_endcurrentBlock = nullNow what happens if there's ANOTHER item after the function_call? Let's say the model outputs a function_call followed by a text message (which can happen with parallel tool calls or follow-up text):
response.output_item.added with function_call → sets currentBlock to toolCallresponse.output_item.done with function_call → does NOT clear currentBlockresponse.output_item.added with message → OVERWRITES currentBlock to text blockSo currentBlock gets overwritten anyway when a new item starts. The missing currentBlock = null isn't actually a bug in the current code flow because each new item overwrites currentBlock.
Wait, but looking at reasoning and message handling in response.output_item.done:
currentBlock = null after completionSo there's an inconsistency, but is it actually a bug? Let me think about what could go wrong if currentBlock still points to the old toolCall block after output_item.done:
The real issue emerges if a delta event arrives without a preceding output_item.added. Events like response.reasoning_summary_text.delta, response.output_text.delta, or response.function_call_arguments.delta all check currentBlock.type to determine where to append data. If currentBlock still references a completed toolCall block and a stray function_call_arguments.delta comes through, it would incorrectly accumulate into the already-finished block instead of being ignored or handled properly.
The PR attempts to address this with a new function_call_arguments.done event. However, examining the delta handler reveals it validates both currentItem?.type === "function_call" and currentBlock?.type === "toolCall". The inconsistency is that after output_item.done fires for a function_call, neither currentItem nor currentBlock gets cleared—unlike the reasoning and message handlers which only clear currentBlock. I need to verify whether the original code actually omits the currentBlock = null reset for function_call or if I misread it.
Looking back at the source, after the toolCall is pushed with toolcall_end, there's no nullification of currentBlock. In contrast, both the reasoning block (which sets currentBlock = null after thinking_end) and message block follow this cleanup pattern. This asymmetry suggests the function_call path may be missing the same reset logic.
The practical risk: if a stray function_call_arguments.delta event somehow arrives after the toolCall completes, the code would still match against the non-null currentBlock and currentItem, potentially appending data to an already-finalized block and emitting spurious deltas. While OpenAI's standard flow shouldn't produce this scenario, the inconsistency leaves the code vulnerable to edge cases or future API changes.
With the PR's updated approach, I need to trace what happens when providers actually send argument deltas. When response.output_item.added fires with a function_call, it initializes currentBlock as a toolCall with empty partialJson. Subsequent function_call_arguments.delta events accumulate the JSON string and attempt to parse it incrementally. Then when response.output_item.done arrives, the code attempts to parse the complete partialJson string to extract the final arguments.
For providers that skip deltas entirely and only send a done event, the flow differs—partialJson starts empty, then the done handler directly assigns the complete arguments string, allowing the final parse to work correctly.
In the unlikely case where both deltas and done events arrive, the done event would overwrite any accumulated delta content, which is acceptable since the done event contains the authoritative complete arguments.
Looking at the currentBlock = null issue more carefully: when response.output_item.done fires for a function call, the code creates a fresh ToolCall object and streams it, but currentBlock was already added to output.content during the initial output_item.added event. This means there are potentially two separate objects—the one stored in the content array and the newly constructed one in the done handler—which could lead to inconsistency if they're not kept in sync.
Comparing this to how reasoning blocks are handled: reasoning updates the existing currentBlock in place, populating its thinking and thinkingSignature fields before emitting. Function calls, by contrast, construct an entirely new object rather than updating the one already in the content array. The currentBlock that was pushed earlier might end up with incomplete or stale data if the arguments aren't properly reflected back into it. The object is pushed to output.content by reference, so mutations during delta events automatically update the stored content. However, when the tool call completes, a new object is created with JSON.parse(item.arguments) instead of using the already-accumulated currentBlock.partialJson. This creates a divergence—the streamed partial JSON built up in currentBlock may differ from what's parsed in the completion event, especially if the provider sends inconsistent data across the stream lifecycle. different object than what was streamed in the deltas. If the JSON parsing was incomplete during streaming, currentBlock.arguments might hold a partial value while item.arguments has the complete parsed result. The PR changes which source is used for the emitted event, but doesn't reconcile this potential mismatch.
For normal providers, the final currentBlock.arguments from the last delta's parse should align with JSON.parse(item.arguments). However, if streaming ended mid-JSON, the partial parse could leave currentBlock.arguments in an inconsistent state. The safest approach would be to explicitly update currentBlock.arguments when output_item.done arrives, ensuring the block's arguments match the final parsed value from the provider.
Looking at the LM Studio flow: the done event sets currentBlock.partialJson to the complete arguments string, but currentBlock.arguments itself is never refreshed after that point. This means output.content[n].arguments remains as whatever the last delta's parse produced, not the final complete value.
Actually, checking the done handler again—it does call parseStreamingJson(currentBlock.partialJson) and assigns that to currentBlock.arguments. So the block gets properly updated in the output.
Back to the original issue: the missing currentBlock = null after output_item.done is technically an inconsistency. It wouldn't cause problems in the normal OpenAI flow since subsequent events trigger output_item.added first. However, with the new function_call_arguments.done handler, a malformed stream could deliver that event after the item completes, causing stale block updates. Setting currentBlock = null acts as a safeguard against this edge case.
Good questions. Let me re-examine.
On currentBlock = null:
Looking at the code flow more carefully:
response.output_item.added overwrites currentBlock with a new blockdelta, done) all check currentBlock?.type === "toolCall" before actingoutput_item.done, the next item would overwrite currentBlock anyway via output_item.addedSo it's not a "preexisting bug" causing actual problems. It's an inconsistency with how reasoning and message handle completion (they set currentBlock = null). Adding it is defensive programming that would only matter if a malformed stream sent a stray function_call_arguments.delta after output_item.done. I overstated it.
On providers that DO support streaming deltas:
Let me trace through:
output_item.added: currentBlock.partialJson = "" (or item.arguments || "")function_call_arguments.delta: partialJson += event.delta, accumulates to complete JSONoutput_item.done: checks currentBlock?.type === "toolCall" && currentBlock.partialJson
currentBlock.partialJson is truthy (accumulated deltas)JSON.parse(currentBlock.partialJson)item.arguments should equal currentBlock.partialJson for conformant providersThe condition currentBlock.partialJson being truthy is key. For delta-streaming providers, it will be populated. For done-only providers (LM Studio), the new handler populates it.
One edge case worth noting: If a provider sends BOTH deltas AND a done event, the done handler would overwrite partialJson with event.arguments. This should be fine since the done event should have the complete value.
Revised verdict: The code is correct, but my characterization of the currentBlock = null as a "preexisting bug" was wrong. It's a stylistic consistency fix / defensive measure.
const args = currentBlock?.type === "toolCall" && currentBlock.partialJson ? JSON.parse(currentBlock.partialJson) : JSON.parse(item.arguments);