Analyzed GitHub PR for tool call argument handlingPublic

Agent

Model

Claude Opus 4.5 (latest)

Started

Jan 24, 2:04 AM

Messages30

Downloads0

Summary

In Progress

Understand
Reviewed full PR details including description, comments, commits, and changed files.
Understand
Understand
Identified and read linked issues referenced in the PR.
Understand
Understand
Analyzed code changes and their impact on tool call argument handling.
Understand
Verify
Clarified 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:

Read the PR page in full. Include description, all comments, all commits, and all changed files.
Identify any linked issues referenced in the PR body, comments, commit messages, or cross links. Read each issue in full, including all comments.
Analyze the PR diff. Read all relevant code files in full with no truncation from the current main branch and compare against the diff. Do not fetch PR file blobs unless a file is missing on main or the diff context is insufficient. Include related code paths that are not in the diff but are required to validate behavior.
Check for a changelog entry in the relevant 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:
- Entry uses correct section (### Breaking Changes, ### Added, ### Fixed, etc.)
- External contributions include PR link and author: Fixed foo ([#123](https://github.com/badlogic/pi-mono/pull/123) by [@user](https://github.com/user))
- Breaking changes are in ### Breaking Changes, not just ### Fixed
Check if packages/coding-agent/README.md, packages/coding-agent/docs/.md, packages/coding-agent/examples/**/.md require modification. This is usually the case when existing features have been changed, or new features have been added.
Provide a structured review with these sections:
- Good: solid choices or improvements
- Bad: concrete issues, regressions, missing tests, or risks
- Ugly: subtle or high impact problems
Add Questions or Assumptions if anything is unclear.
Add Change summary and Tests.

Output format per PR: PR: Changelog:

... Good:
... Bad:
... Ugly:
... Questions or Assumptions:
... Change summary:
... Tests:
...

If no issues are found, say so under Bad and Ugly.

Let me read the full source file to understand the context:

Let me check for any related tests and look at the parseStreamingJson function to understand its behavior better:

Now let me check if documentation needs updates:

PR: https://github.com/badlogic/pi-mono/pull/917

Changelog:

Missing. No entry in packages/ai/CHANGELOG.md under [Unreleased]. Required before merge.

Good:

Addresses a real bug with LM Studio hosting GLM-4.7-Flash (and potentially other OpenAI-compatible providers) that send tool call arguments via response.function_call_arguments.done instead of streaming deltas
The fix correctly prefers currentBlock.partialJson (populated from done event) over item.arguments (which is empty "{}" in this case)
Adds missing 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)
The parseStreamingJson function tries JSON.parse first, so using it on complete JSON from the done event is correct
Clear PR description with reproduction steps and example config

Bad:

No tests added. This edge case should have a test to prevent regression (e.g., mock SSE stream that sends function_call_arguments.done without deltas)
Contribution process not followed. The bot already flagged this: contributor needs to open an issue first per CONTRIBUTING.md. The PR will be auto-closed.
Minor: Using parseStreamingJson in the done handler but JSON.parse in the completion handler is inconsistent (both work, but mixing styles)

Ugly:

Nothing severe. The fix is straightforward and isolated.

Questions or Assumptions:

Is GLM-4.7-Flash the only model exhibiting this behavior, or are there other LM Studio models that do this? (Answer may affect testing scope)
Does the contributor have other models they've tested against to verify no regression?

Change summary:

Added handler for response.function_call_arguments.done event that stores complete arguments in currentBlock.partialJson
Modified response.output_item.done handler for function_call to prefer pre-parsed arguments from currentBlock.partialJson over potentially empty item.arguments
Added missing currentBlock = null cleanup after function_call completion (bug fix unrelated to the main issue)

Tests:

None added. Should add a test with a mock OpenAI Responses stream that sends function_call_arguments.done without prior delta events.

Verdict: The code looks correct and the fix is sensible, but the contributor needs to:

Open an issue first (per contribution guidelines)
Add a test case for this behavior
Add a changelog entry

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?

Good questions. Let me re-examine.

On currentBlock = null:

Looking at the code flow more carefully:

Each response.output_item.added overwrites currentBlock with a new block
The event handlers (delta, done) all check currentBlock?.type === "toolCall" before acting
After output_item.done, the next item would overwrite currentBlock anyway via output_item.added

So 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 || "")
Multiple function_call_arguments.delta: partialJson += event.delta, accumulates to complete JSON
output_item.done: checks currentBlock?.type === "toolCall" && currentBlock.partialJson
- currentBlock.partialJson is truthy (accumulated deltas)
- Uses JSON.parse(currentBlock.partialJson)
- item.arguments should equal currentBlock.partialJson for conformant providers
- Result: same behavior, no regression

The 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.