Skip to content

fix: preserve tool_use stop reason for Responses function calls - #173

Merged
samueltuyizere merged 7 commits into
routatic:mainfrom
hinha:fix/responses-tool-use-stop-reason
Sep 11, 2026
Merged

fix: preserve tool_use stop reason for Responses function calls#173
samueltuyizere merged 7 commits into
routatic:mainfrom
hinha:fix/responses-tool-use-stop-reason

Conversation

@hinha

@hinha hinha commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Summary

Fix Claude Code tool-call compatibility for OpenCode Go Responses models.

This PR addresses two related response-conversion problems:

  1. Responses function_call output was converted to Anthropic tool_use content while retaining stop_reason: "end_turn". The proxy now emits stop_reason: "tool_use" when a function call is present.
  2. Some provider-generated tool arguments contained whitespace-suffixed JSON keys such as "description ". Claude Code Plan Mode rejects these as Invalid tool parameters because the expected schema key is description. The proxy now normalizes whitespace around JSON object keys in both non-streaming and streaming Responses tool calls before sending them to Claude Code.

The second issue was reproduced from a Claude Code 2.1.263 transcript:

InputValidationError: AskUserQuestion failed
questions[1].options[0].description: expected string, got unknown

The malformed input was:

{"label":"Repo + Wiki + Ask","description ":"..."}

Validation

  • GOTOOLCHAIN=local /usr/local/go/bin/go test -count=1 ./...
  • GOTOOLCHAIN=local /usr/local/go/bin/go build ./...
  • git diff --check
  • Regression coverage for normalized and streaming Responses tool arguments

Closes #172

Source branch: hinha:fix/responses-tool-use-stop-reason
Latest commit: 29cf114

@kilo-code-bot

kilo-code-bot Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Code Review Roast 🔥

Verdict: No Issues Found | Recommendation: Merge

Oh wait, this PR is actually clean. I need to sit down. I had my flamethrower warmed up and everything.

📊 Overall: Like finding a unicorn in production — I didn't think clean PRs existed anymore, but here we are.

Files Reviewed (5 files)
  • AGENTS.md
  • internal/transformer/stream.go
  • internal/transformer/stream_test.go
  • internal/transformer/tool_arguments.go
  • internal/transformer/tool_arguments_test.go
Previous Review Summaries (3 snapshots, latest commit 8657cb2)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 8657cb2)

Verdict: No Issues Found | Recommendation: Merge

Oh wait, this PR actually fixed the issues. I need to sit down. I had my flamethrower warmed up and everything.

📊 Overall: Like finding a unicorn in production — I didn't think clean PRs existed anymore, but here we are.

Files Reviewed (3 files)
  • internal/transformer/response.go - 0 issues (previous issue resolved)
  • internal/transformer/tool_arguments.go - 0 issues (previous issue resolved)
  • internal/transformer/tool_arguments_test.go - 0 issues

Previous review (commit 27595d5)

Verdict: 2 Issues Found | Recommendation: Address before merge

Overview

Severity Count
🚨 critical 0
⚠️ warning 2
💡 suggestion 0
🤏 nitpick 0
Issue Details (click to expand)
File Line Roast
internal/transformer/tool_arguments.go 22 json.Marshal escapes HTML characters in tool argument values, silently corrupting strings containing <, >, or &
internal/transformer/response.go 203 Missing normalizeToolArguments call in non-streaming Responses path — the PR promised both paths but forgot this one

🏆 Best part: The streaming path now correctly buffers and normalizes tool arguments, and the hasToolCall stop-reason logic is properly mirrored across both Responses paths.

💀 Worst part: response.go was left behind like the last kid picked for dodgeball — the non-streaming Responses path still sends raw malformed arguments, directly contradicting the PR's promise of fixing "both non-streaming and streaming."

📊 Overall: This PR built a solid boat for the streaming river but left a hole in the non-streaming hull. The HTML-escaping bug in tool_arguments.go is the iceberg nobody saw coming.

Files Reviewed (7 files)
  • internal/transformer/normalized_bridge.go - 0 issues
  • internal/transformer/normalized_bridge_responses_test.go - 0 issues
  • internal/transformer/response.go - 1 issue
  • internal/transformer/response_test.go - 0 issues
  • internal/transformer/stream.go - 0 issues
  • internal/transformer/stream_test.go - 0 issues
  • internal/transformer/tool_arguments.go - 1 issue

Fix these issues in Kilo Cloud

Previous review (commit 7e6dcc9)

Verdict: No Issues Found | Recommendation: Merge

Oh wait, this PR is actually clean. I need to sit down. I had my flamethrower warmed up and everything.

📊 Overall: Like finding a unicorn in production — I didn't think clean PRs existed anymore, but here we are.

Files Reviewed (2 files)
  • internal/transformer/normalized_bridge.go - 0 issues
  • internal/transformer/normalized_bridge_responses_test.go - 0 issues

Reviewed by free · Input: 0 · Output: 0 · Cached: 0

Review guidance: REVIEW.md from base branch main (truncated)

@alecchen

Copy link
Copy Markdown
Contributor

Same hardcoded "end_turn" at internal/transformer/response.go:225 as normalized_bridge.go:272 had. It is still reachable through the fallback in messages.go:1486. Can you apply the same hasToolCall logic here so both Responses paths return "tool_use"?

I am hitting this early stop with muse-spark-1.3-contributor. A tool call comes back and Claude stops instead of running it. This looks like the right fix to me.

@hinha

hinha commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

Applied in internal/transformer/response.go — you were right that the hardcoded end_turn was still there while function_call outputs were already being converted into tool_use blocks. That inconsistency is fixed now, with a regression test covering both cases (tool call → tool_use, text only → end_turn).

Some context from my side, and I'd like to keep this as an open conversation.

Before your comment I had already tried several different approaches specifically for muse-spark-1.3-contributor — different wire formats, keeping the model ID bare, different routing — and the early stop only ever showed up with Muse. Other models, for example the whole DeepSeek V4 series, are fine with the same changes and never end a turn early after a tool call. So in my testing this looks model-specific rather than something in the shared Responses path.

That is also why I would rather check the reachability question with you than assume this closes your case. In the current code, TransformResponsesResponse is only reachable through the legacy branch in messages.go (the "provider not found in registry, falling back to old client" one). server.go always registers opencode-go, opencode-zen and aws-bedrock, and for any provider that is not registered client.IsZen(model) is false — so the EndpointResponses arm is never selected in a normal build. My read: on a build from main, the live Responses path was ResponsesToNormalized (the one this PR fixes), and the end_turn you spotted in response.go is a genuine latent inconsistency rather than the one your Muse early stop came from.

So two things that would help:

  1. Which build did you hit it on — a release, or main?
  2. If you still see it after this patch, could you share a Muse turn with the raw SSE events (response.output_item.added, response.output_item.done, response.completed)? That would quickly tell us whether the function call arrives without an added event, which is the other way a tool call can end up with the wrong stop reason.

Happy to dig into it together.

TransformResponsesResponse converted a Responses function_call into a
tool_use block but always reported stop_reason "end_turn", so a client
that read the turn as finished skipped the tool call.

Mirror the hasToolCall logic already used by ResponsesToNormalized so
both Responses paths report "tool_use".

Co-Authored-By: Claude Code <noreply@anthropic.com>
@hinha

hinha commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

Update — the fix is now actually in this branch, not just described: 27595d5 (fix(transformer): keep tool_use stop reason on Responses fallback path).

What changed: TransformResponsesResponse now tracks whether any function_call output was converted, and reports stop_reason: "tool_use" when it was. That is the same hasToolCall logic ResponsesToNormalized already uses, so both Responses paths agree.

Verification:

  • go test -count=1 ./... — passes
  • go build ./..., go vet ./internal/transformer/, gofmt -l — clean
  • New regression test TestTransformResponsesResponseStopReason covers both directions (function_calltool_use, text only → end_turn). On the previous commit it fails with StopReason = "end_turn", want "tool_use"; with this commit it passes.

@alecchen the line number you gave was exact — response.go:225 on the current branch. Still interested in the two points from my earlier comment, because I would rather confirm your repro than assume this closes it: which build you hit the early stop on (a release, or main), and if it still happens with this commit, a Muse turn with the raw SSE events (response.output_item.added, response.output_item.done, response.completed). That would tell us whether the function call reaches the proxy at all, or arrives in a shape we do not turn into a tool_use block yet.

Comment thread internal/transformer/tool_arguments.go Outdated
Comment thread internal/transformer/response.go Outdated
martinusN and others added 2 commits September 11, 2026 09:19
normalizeToolArguments is documented as repairing whitespace around JSON
object keys, but it decoded into any and re-encoded, which rewrote the
values as well:

  {"id":1234567890123456789} -> {"id":1234567890123456800}
  {"price":1.0}              -> {"price":1}
  {"html":"<b>bold</b>"}     -> the < and > inside the value are escaped

Integers above 2^53 lose precision, which silently corrupts a tool
argument that carries a large ID or byte count. Decode with UseNumber so
number literals survive, and disable HTML escaping on the encoder so
string values are re-emitted untouched.

A Decoder also accepts the trailing content that json.Unmarshal rejected
({"a":1}{"b":2}), which would silently keep only the first value; that
input is now returned unchanged, as before.

Co-Authored-By: Claude Code <noreply@anthropic.com>
…path

TransformResponsesResponse passed output.Arguments straight through,
unlike ResponsesToNormalized, so the key-whitespace repair added for
Responses tool calls did not cover the non-streaming fallback path.

Co-Authored-By: Claude Code <noreply@anthropic.com>
@hinha

hinha commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

Both items are addressed, in 071fe7e and 8657cb2.

response.go — correct, applied

TransformResponsesResponse now passes output.Arguments through normalizeToolArguments, so the non-streaming fallback path matches ResponsesToNormalized.

tool_arguments.go — the observation is right, the impact is not, and something worse was hiding in the same two lines

Reproduced both cases: {"html":"<b>bold</b>"} does come back with the < and > emitted as unicode escape sequences, and {"amp":"a&b"} does the same for &. That part of the report holds.

What does not hold is the consequence. A unicode escape is the character it encodes — < and < parse to the same string. Claude Code parses the JSON before validating it, so no schema validator ever sees a different value, and the tool call was never rejected because of this.

What the same round-trip genuinely breaks is numbers, because the decode goes through float64:

{"id":1234567890123456789} -> {"id":1234567890123456800}
{"n":9007199254740993}     -> {"n":9007199254740992}
{"price":1.0,"exp":1e21}   -> {"exp":1e+21,"price":1}

An argument carrying a large ID, a byte count or a nanosecond timestamp is silently corrupted — no error, no log, just a different number. That is a real defect, and it lives in the exact two lines the review pointed at, so both are fixed together:

  • json.Decoder with UseNumber() keeps number literals exactly as the provider sent them
  • json.Encoder with SetEscapeHTML(false) stops rewriting string values
  • decoding with a Decoder would also have accepted trailing content that json.Unmarshal rejected, silently keeping only the first value of {"a":1}{"b":2}; that input is returned unchanged, as it was before

New tool_arguments_test.go covers key trimming, number precision, escaping, invalid JSON, trailing content and the exact-key-wins collision case.

Verification: go build ./..., go vet ./internal/transformer/ and gofmt -l clean, go test -count=1 ./... passes.

@alecchen

Copy link
Copy Markdown
Contributor

Thanks @hinha, checked at head - TransformResponsesResponse now matches ResponsesToNormalized (hasToolCall + normalizeToolArguments), with both directions covered by tests. That closes the line I flagged.

Agree on reachability. I read messages.go:1290-1314 and server.go:71-75: normal builds go through provider dispatch into ResponsesToNormalized, and the function I flagged only runs on the legacy provider not found in registry + Zen EndpointResponses arm. Claude Code also streams, so my early stop would have gone through ProxyResponsesStream, which already had the hasToolUse gate. This reads like a real latent bug worth fixing, just not the path my repro hit.

I also tried forwarding reasoning.effort in NormalizedToResponses on a local branch. Muse still stops early with it, so that is not the cause either. Same as you saw: DeepSeek V4 is fine, only Muse shows this.

If it comes back after this patch I will grab one Muse turn of raw SSE (response.output_item.added, response.output_item.done, response.completed) and post it here.

@samueltuyizere

Copy link
Copy Markdown
Collaborator

Thank you all for working on this. I am going through it and will revert shortly!

@samueltuyizere
samueltuyizere self-requested a review September 11, 2026 09:20

@samueltuyizere samueltuyizere left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks again @hinha !
@alecchen lmk if you are still having issues with muse and kindly attach detailed request logs. else, I'd go ahead and release this.

@alecchen

Copy link
Copy Markdown
Contributor

LGTM

@samueltuyizere
samueltuyizere merged commit 3abc2e1 into routatic:main Sep 11, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: preserve tool_use stop reason for Responses function calls

4 participants