Repository navigation
fix(llm): preserve opaque tool-call fields across OpenAI-compatible turns - #1233
basil-k-aji-dev wants to merge 5 commits into
Conversation
…urns Some providers attach metadata to an assistant tool call that must be echoed unchanged on the next request. Vertex AI's Gemini endpoint returns a thought signature under extra_content and rejects the following request with HTTP 400 when it is missing, so a review died on its second turn. Capture the tool-call fields OCR does not model and replay them via the SDK's SetExtraFields. id, type and function stay owned by OCR, malformed payloads are dropped rather than forwarded, and the opaque data is json:"-" so it stays out of review and session output. Fixes alibaba#947
|
🔍 OpenCodeReview found 1 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if !json.Valid(v) { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The json.Valid(v) check is redundant here. When json.Unmarshal decodes into map[string]json.RawMessage, the decoder must fully parse each value to locate token boundaries — if any value were invalid JSON, the Unmarshal on line 280 would have returned an error. This means every json.Valid call re-parses bytes that are already guaranteed valid, adding unnecessary CPU cost proportional to the number and size of extra fields on every response mapping.
Removing this check is safe and eliminates the redundant work:
There was a problem hiding this comment.
Good catch — fixed in 468f90a.
I checked the claim before acting on it rather than assuming: json.Unmarshal into map[string]json.RawMessage has to scan each value to find its boundaries, so a malformed inner value fails at the Unmarshal on line 280 and never reaches the loop. Confirmed against {"a":{invalid}}, {"a":{"b":}}, {"a":tru} and {"a":} — all error at Unmarshal.
Removed the len(v) == 0 guard in the same commit, since it is dead for the same reason: a value in a valid document is always at least one byte (null is four). Left a comment on the Unmarshal saying retained values are valid by construction, so the next reader does not re-add either check.
The malformed-payload behaviour the issue asks for is unchanged — it comes from the Unmarshal error path, and TestOpaqueToolCallFields_MalformedNotForwarded still covers it.
|
Independent verification on a second Gemini surface: the AI Studio endpoint Minimal repro against the endpoint, no OCR involved. Turn 1 returns a tool call carrying Re-sending the identical request with OCR, same repository and diff both times — one 20-line Python file with a planted SQL injection, a
Two notes that may be useful:
Related: #1042 is what makes this hard to diagnose in the first place — the 400 surfaces with an empty body, so the message quoted above never reaches the user. Disclosure: these verification runs were driven with an AI coding assistant (Claude Opus 5). Every output quoted above is from a real run against the live endpoint. |
|
I tested commit The test makes two requests. The first streamed response returns a tool call with I tried putting It looks like the field gets lost in Could we cover the streaming path in this PR as well? I have the focused test and a small patch ready if that helps. |
Gemini's OpenAI-compatible endpoint rejects a follow-up turn that does not echo the opaque fields it attached to a tool call, so every review this provider ran ended as a failed subtask and the work fell through to Groq and then OpenAI. alibaba/open-code-review#1233 fixes it in internal/llm/client.go and is not in a release. Pinning that pull request through uses: would change nothing. The action is composite and installs the npm package, a launcher that downloads a published binary, so a fork of the action runs the identical engine. The job builds the engine instead and prepends it to the path, where it answers ocr ahead of the shim npm installs. The action, including its own install step, is untouched. Both new steps continue on error, so a build that breaks degrades to the published binary rather than taking the provider chain down. Base commit and fix commit are pinned by sha: the fix sits on a fork branch that can be force-pushed. Delete both steps once npm ships a version past 1.12.4 carrying the fix.
The non-streaming capture reads tc.RawJSON() on the completed response. ChatCompletionAccumulator builds its ChatCompletion field by field and never sets raw JSON on the assembled message or its tool calls, so on the streaming path that string is empty and opaqueToolCallFields returns nil: Vertex AI's thought signature was still dropped and the next request still rejected with HTTP 400. Capture the unmodelled fields from the tool-call delta instead, where they still exist, keyed by choice and tool index and merged across chunks since a provider may attach the metadata to any chunk of the call. Reattach after the accumulated response is mapped. JSON.ExtraFields holds only what the SDK does not model, so the stream's own "index" framing never reaches the next request, and the reserved filter still applies so a provider cannot rewrite a tool call's identity on either path. Reported by @subaru-ye on alibaba#1233. Co-authored-by: subaru-ye <178248423+subaru-ye@users.noreply.github.com>
The non-streaming capture reads tc.RawJSON() on the completed response. ChatCompletionAccumulator builds its ChatCompletion field by field and never sets raw JSON on the assembled message or its tool calls, so on the streaming path that string is empty and opaqueToolCallFields returns nil: Vertex AI's thought signature was still dropped and the next request still rejected with HTTP 400. Capture the unmodelled fields from the tool-call delta instead, where they still exist, keyed by choice and tool index and merged across chunks since a provider may attach the metadata to any chunk of the call. Reattach after the accumulated response is mapped. JSON.ExtraFields holds only what the SDK does not model, so the stream's own "index" framing never reaches the next request, and the reserved filter still applies so a provider cannot rewrite a tool call's identity on either path. Reported by @subaru-ye on alibaba#1233.
|
@subaru-ye You're right, and thanks for the precise diagnosis — it saved me the search. Reproduced both of your cases and fixed on the streaming path in a0c9ef1. Reproduction. A local SSE server, Root cause — one step sharper than "lost in the accumulator". I probed the SDK types directly. The delta does carry it:
The fix, matching the
Test added as One honest caveat: I could not run Please do still post your focused test. If it covers a placement mine misses I would rather carry yours than assume two cases are the whole space, and you found this in the first place. @lizhengfeng101 this now covers streaming as well as the non-streaming path, so #947 should be fully closed by it rather than half. Happy to split the streaming commit into its own PR if you would prefer to review them separately. |
|
Verified I also ran my focused test against this commit. The full HTTP round-trip cases (non-streaming / metadata on the opening chunk / metadata on a later chunk) all pass. One case behaves differently from my local patch, though: when the delta that opens a tool call carries a negative tool index, the capture is keyed by that raw index ( {"choices":[{"index":0,"delta":{"tool_calls":[{"index":-1,"id":"call_1","type":"function","function":{"name":"file_read","arguments":"{}"},"extra_content":{"google":{"thought_signature":"sig"}},"provider_flag":true}]}},{"index":0,"delta":{"tool_calls":[{"index":0,"extra_content":{"google":{"thought_signature":"replacement"}}}]},"finish_reason":"tool_calls"}]}The replayed request carries only |
The capture keyed by the delta's raw index while the attach loop reads the accumulator's slice position. Those agree only after clamping: the SDK maps a negative index to 0, with the comment that "the API may send -1 for single tool calls". A signature on the chunk that opens a single call therefore landed in bucket -1 and was never attached - the one chunk where providers are most likely to put it. Key by the same clamped index the accumulator uses, so capture and attach address the same call. Reported by @subaru-ye on alibaba#1233. Co-authored-by: subaru-ye <178248423+subaru-ye@users.noreply.github.com>
The capture keyed by the delta's raw index while the attach loop reads the accumulator's slice position. Those agree only after clamping: the SDK maps a negative index to 0, with the comment that "the API may send -1 for single tool calls". A signature on the chunk that opens a single call therefore landed in bucket -1 and was never attached - the one chunk where providers are most likely to put it. Key by the same clamped index the accumulator uses, so capture and attach address the same call. Reported by @subaru-ye on alibaba#1233.
|
@subaru-ye Thank you for the race run — and you found a real bug, not an edge case I can wave off. I went to check whether a negative index is something providers actually send, and the answer is in the SDK's own accumulator: // Clamp negative indices to 0 since the API may send -1 for single tool calls
toolIndex := clampToZero(deltaTool.Index)
choice.Message.ToolCalls = expandToFit(choice.Message.ToolCalls, toolIndex)So your clamp is not a defensive guess, it is the accumulator's documented rule — and "single tool calls" is precisely the common case. My capture keyed by the raw Fixed in 8b4f040 — key by the same clamped index the accumulator uses. Added your payload as a third subtest,
Please do post your focused test. Two of my three subtests now come from you, which is a decent argument that your case coverage is better than mine. @lizhengfeng101 no rush on your side — this is settling rather than churning, and I'd rather it land correct than fast. |
|
Verified |
Qiyuanqiii
left a comment
Qiyuanqiii
left a comment
There was a problem hiding this comment.
Thanks for continuing to refine this — I went through the current head (8b4f040) again, including the non-streaming and streaming paths and the follow-up fixes from the discussion.
The implementation looks solid to me at this point. In particular, the streaming metadata capture, later-chunk handling, and the -1 tool index fix now line up well with openai-go's accumulator behavior. The reserved-field filtering also makes the generic ExtraFields approach reasonably contained, and the independent race-test verification is reassuring. I don't see an obvious code-level blocker in the current revision.
One small hardening thought, not something I would block on: ExtraFields is a map[string]json.RawMessage, while NewToolCallMessage and internal/session.copyMessages still copy ToolCall values shallowly. That means those copies share the same map. I couldn't find a current path that mutates it after capture, so this looks safe with the existing immutable-use pattern, but a deep-copy helper/test could make that assumption explicit in the future.
There is also now some overlap with #1393. I don't think the two PRs are strict duplicates: this PR takes the more general approach of preserving arbitrary opaque tool-call fields, while #1393 takes a narrower extra_content approach and also adds provider-error diagnostics and session persistence.
Because those are genuinely different design choices, I don't think it would be fair to decide between them simply based on which PR lands first. I'd be interested in maintainer guidance on which abstraction the project prefers, and whether the generic approach here should remain the direction for #947 or be reconciled with #1393 in some other way.
From my side, though, the current #1233 implementation itself looks technically sound.
|
@subaru-ye a question about attribution on this PR, separate from the scope discussion. Both of the streaming commits exist because of your work. That is co-authorship, and the commits should say so rather than leaving it in a thread. If you are willing, I would like to add: Two things I would rather ask than assume. First, whether you want it at all — some people prefer not to be on commits they did not write. Second, which address: the trailer only attaches to an account if the email matches one GitHub knows, so I would rather you gave me the one you want than have me guess an id and produce a line that credits nobody. If you would rather not, that is fine and I will leave the commits alone; the thread already records what you did. And if a maintainer lands #1371 or #1393 instead of this PR, the same offer stands for whoever carries the change — @Moon-Wrecker has already said they would add trailers for both of us. No rush, and no need to answer if the scope question is settled first. |
|
Yes please, I'd like the attribution. |
8b4f040 to
8f8701f
Compare
|
Done — added to both commits, which are the two your work produced: Only the messages changed; Thank you for the address rather than making me guess an id. A trailer whose email GitHub does not recognise renders as plain text and credits nobody, which is a quiet way to lose exactly the attribution it is meant to record. The offer still stands for whoever carries this if the maintainers land #1371 or #1393 instead — @Moon-Wrecker has already said they would add trailers for both of us, and @AyushDubey23 has posted theirs on that thread. |
Raised in review by @Qiyuanqiii. ExtraFields is a map, and both copy sites — NewToolCallMessage and session.copyMessages — copied ToolCall values with a plain slice copy, so every copy shared the one map and the byte slices in it. Nothing mutates the map after mapOpenAIResponse fills it, so this was safe. But safe by a convention held in two packages with nothing to enforce it: a future write through any copy would reach the original, and the failure would surface as a tool call carrying another turn's signature rather than as anything local to the writer. CloneToolCalls copies the map and the values inside it. The map is small and tool calls are few, so making the type safe by construction costs less than keeping the convention correct. Tests pin both halves — a new key written through the clone, and an in-place overwrite of the shared backing array.
6af5822 to
058d0cb
Compare
|
@Qiyuanqiii thank you — and the shallow-copy note was worth acting on, so I have. You were right on every point. Both sites copy shallowly: // NewToolCallMessage
copy(tc, toolCalls)
// session.copyMessages
ToolCalls: append([]llm.ToolCall(nil), m.ToolCalls...),and the only write to a tool call's Fixed in 058d0cb with Two tests pin it, and both fail against the previous revision: The first writes a new key through the clone; the second overwrites the shared byte slice in place. A third checks nil and empty input stay nil, so a tool call with no opaque fields does not gain an empty map.
On the overlap with #1393, I agree with your framing and have nothing to add beyond what I wrote there: the generic abstraction is the one I would argue for, the persistence choice in #1393 is the better one and I would drop mine, and which PR lands is entirely the maintainers' call. @AyushDubey23 reached the same conclusion on that thread. I would rather the Gemini bug were fixed this week than that this is the PR that fixes it. |
|
@NanaseInori review |
NanaseInori
left a comment
NanaseInori
left a comment
There was a problem hiding this comment.
Re-reviewed the latest head (058d0cb). The remaining hardening concern from my previous review has now been addressed cleanly.
CloneToolCalls deep-copies both the ExtraFields map and each json.RawMessage backing byte slice, and it is used by both NewToolCallMessage and internal/session.copyMessages. The new mutation tests make the intended no-aliasing property explicit rather than relying on an immutable-use convention.
The earlier streaming fixes also remain sound: opaque fields are captured before ChatCompletionAccumulator drops them, merged across chunks, keyed with the same negative-index clamping behavior as the SDK, and guarded against attaching metadata to the wrong tool-call ID. Reserved fields remain owned by OCR, while provider-specific fields are replayed generically.
I don't see a remaining code-level blocker in this revision. The overlap with #1393 is now a maintainer-level question of scope/merge sequencing rather than a correctness objection to this implementation.
This review was conducted by NanaseInori's review bot, using the model GPT-5.6 Sol. If you need a human review, please manually @NanaseInori.
|
Thanks for this, @basil-k-aji-dev — and sorry for the overlap. Closing as superseded by #1393, which has now been merged and resolves #947. It preserves the provider's The one design difference: your version preserves every unmodeled tool-call field through a generic If you later hit a provider that stashes required metadata under a different key, that generalization would be a welcome follow-up — please open a fresh PR for it and reference this discussion. Thanks again for the careful work. |
Description
Some OpenAI-compatible providers attach opaque metadata to an assistant tool call that must be echoed unchanged on later turns. Vertex AI's OpenAI-compatible Gemini endpoint returns a thought signature under
tool_calls[].extra_content.mapOpenAIResponsemapped the SDK response onto OCR'sToolCall, which modelled onlyid,typeandfunction, so everything else was dropped.buildOpenAIParamsthen rebuilt the assistant history without it and the provider rejected the next request with HTTP 400.This breaks ordinary repository tools: turn one can call
file_read, and turn two fails before the model can continue the review.Change
ToolCallgainsExtraFields map[string]json.RawMessage, captured verbatim fromtc.RawJSON()during response mapping.buildOpenAIParamsreplays them through the SDK'sSetExtraFields, the same mechanism already used forreasoning_contentin Thinking state is parsed but dropped when assistant tool-call turns are replayed #805.Four properties the issue asks for, and how each is held:
SetExtraFieldsoverrides same-key fields, soid,typeandfunctionare stripped at capture time. A provider cannot use this channel to rewrite the identity of a tool call.nilrather than a partial map. Forwarding half a payload is worse than dropping it, since the provider would reject the request either way.json:"-". Session logs already build an explicitid/name/argumentsmap ininternal/session/history.go, so they were never a leak path, but the tag closes the struct-marshalling one.json.RawMessagekeeps the subtree verbatim rather than round-tripping through a typed shape.Only the OpenAI chat-completions path is touched. The Anthropic mapping builds
ToolCallfrom typedtool_useblocks with no raw payload to carry, so it is unaffected.Type of Change
How Has This Been Tested?
make testpasses locallyAdded four tests in
internal/llm/native_turn_test.go, alongside the existingreasoning_contentreplay tests:TestOpenAIChatCompletions_ReplaysOpaqueToolCallFieldsAcrossTurns— the end-to-end adapter regression the issue asks for. It maps a response carryingextra_content.google.thought_signature, rebuilds the assistant history, and asserts the nested subtree appears in the next request whileidandfunction.nameremain OCR's.TestOpaqueToolCallFields_ReservedFieldsStayOurs— a response claimingid/type/functioncannot capture them.TestOpaqueToolCallFields_MalformedNotForwarded— empty, truncated, non-object and non-JSON payloads all yield nothing.TestToolCall_OpaqueFieldsStayOutOfSerializedOutput— the opaque payload does not appear in marshalled output.I verified the end-to-end test actually catches the defect rather than passing by construction. With the capture line removed it fails with exactly the behaviour from the report:
make check(license and english-check included) and the full package list pass.One environment note, for accuracy:
make testruns with-race, and the race detector cannot start on my arm64 machine (ThreadSanitizer: unsupported VMA range, Found 47 - Supported 48) for any package, onmainas well as on this branch. I ran the same package list withLC_ALL=C go test -count=1; all packages pass. CI covers the-racepath.I do not have Vertex AI access, so the live 400 is not something I can reproduce first-hand. The tests are built on the provider-independent reproduction described in the issue.
Checklist
go fmt,go vet)AI disclosure: developed with AI assistance (Claude Opus 5, via Claude Code). I reviewed the change and the reasoning myself, no commits are attributed to AI, and I will answer maintainer questions in my own words.
Related Issues
Closes #947