fix(llm): replay tool_call extra_content and surface provider error bodies - #1393

All committers have signed the CLA.

🔍 OpenCodeReview found 1 issue(s) in this PR.

• ✅ Successfully posted inline: 1 comment(s)

Hi @chaojixinren , @wu21-web , @austinuni60-netizen , @Qiyuanqiii , @basil-k-aji-dev , can you kindly review this PR.

No big deal. I’m planning to go through all open PRs starting this afternoon

Please do not tag individual users for reviews. Somebody will review it for you. I will review for you soon, but please use ocr first.

@Moon-Wrecker Before reviewing, I suggest splitting changes up into different PRs that are small and focused. Big chunks make it hard to review. Be aware that this closes 3 issues on merge, I don't really recommend that ..

Thanks @Moon-Wrecker for acknowledging the prior work and design from #1371 in the credit section.

I agree with @wu21-web and @Qiyuanqiii regarding scope, PR size, and contribution lineage. Bundling three distinct issues (#1357, #1042, #947) along with session persistence refactors into a 700+ line PR creates a large blast radius and complicates review.

For the concrete bug in #1357 and #1042, PR #1371 is already open, tested, and strictly focused on solving the extra_content replay and provider error surfacing without expanding the scope.

Landing the focused fix in #1371 resolves the immediate blocker for Gemini users cleanly, while the broader architectural discussions around #947 and #1233 can proceed on their own timeline.

100% agree with this principle, @Qiyuanqiii. “Prefer the smallest complete fix that can be explained, validated, and attributed independently” makes a lot of sense here.

Just to be completely transparent about the lineage here:

Right after I opened #1371 to address #1357, @Moon-Wrecker asked on that issue whether my fix also covered #1042. Shortly after that, #1393 was opened, lifting the exact implementation details from #1371, including the negative delta index clamping, the provider neutral error body enrichment for array wrapped Gemini payloads, and the 64KB log bounding, and combining them with @basil-k-aji-dev’s work from #1233 into one mega PR attempting to close all 3 issues at once.

Subsuming active PRs from other contributors into a 700+ line PR without collaboration isn’t really great open source etiquette, and as @wu21-web also pointed out, it makes the review process much harder.

I think the cleanest way forward, while keeping attribution clear and the changes easy to review, would be:

1. fix(llm): preserve opaque tool-call fields across OpenAI-compatible turns #1233 by @basil-k-aji-dev merges to solve the generic tool call metadata replay issue (fix(llm): preserve opaque tool-call fields across OpenAI-compatible turns #947).
2. fix(llm): preserve tool_call extra_content and enrich error body for OpenAI providers #1371 by @AyushDubey23 handles the provider error body enrichment issue (Opaque "400 Bad Request" error with missing details when using Gemini / OpenAI-compatible endpoints #1042).

Both PRs are already tested and independently scoped, and this avoids the unnecessary blast radius of #1393 while keeping each fix focused and properly attributable.

@Moon-Wrecker As a maintainer, I still recommend keeping these changes within a single PR; since they significantly impact the overall review, consolidating them makes it easier for me to conduct an end-to-end evaluation. From this perspective, I wonder if anyone holds a different view. cc @wu21-web @Qiyuanqiii @AyushDubey23

Thanks for looping me in, @lizhengfeng101.

I get the end to end evaluation point. #1357 and #1042 are closely connected when testing live Gemini/Vertex endpoints, since the enriched error body makes tool call failures much easier to debug. That’s why I paired both fixes in #1371.

I think we can address both concerns pretty cleanly:

1. fix(llm): preserve tool_call extra_content and enrich error body for OpenAI providers #1371 already covers both fixes in ~270 lines, mostly tests, without the 700+ line session persistence changes.

2. If a consolidated PR is preferred, I’m happy to collaborate. I’d just ask that the existing contributions from @basil-k-aji-dev, @subaru-ye and myself are preserved through proper git attribution such as Co-authored-by: trailers, along with tests for all affected paths.

Happy to go with whichever direction the maintainers prefer.

Thanks for looping me in, @lizhengfeng101.

I get the end to end evaluation point. #1357 and #1042 are closely connected when testing live Gemini/Vertex endpoints, since the enriched error body makes tool call failures much easier to debug. That’s why I paired both fixes in #1371.

I think we can address both concerns pretty cleanly:

1. fix(llm): preserve tool_call extra_content and enrich error body for OpenAI providers #1371 already covers both fixes in ~270 lines, mostly tests, without the 700+ line session persistence changes.
2. If a consolidated PR is preferred, I’m happy to collaborate. I’d just ask that the existing contributions from @basil-k-aji-dev, @subaru-ye and myself are preserved through proper git attribution such as Co-authored-by: trailers, along with tests for all affected paths.

Happy to go with whichever direction the maintainers prefer.

I strongly agree with recognizing the high-quality contributions of existing developers through Git attribution.

@Moon-Wrecker As a maintainer, I still recommend keeping these changes within a single PR; since they significantly impact the overall review, consolidating them makes it easier for me to conduct an end-to-end evaluation. From this perspective, I wonder if anyone holds a different view. cc @wu21-web @Qiyuanqiii @AyushDubey23

Thanks, that clarifies the PR-organization question for me. I'm fine with treating #1393 as the consolidated integration PR if that makes the end-to-end review easier.
I think one architectural question is still unresolved, though: which tool-call metadata abstraction should the consolidated implementation use?
#1233 preserves arbitrary unmodeled provider fields through ExtraFields, while #1393 currently preserves only the known extra_content field through ExtraContent. There is also a related persistence difference: #1233 keeps that opaque state out of the session JSONL, while #1393 deliberately persists it for replay/export.
Keeping the work in one PR doesn't by itself resolve those choices. Before treating #1233 as superseded, could you clarify which behavior you want the project to standardize on?
I'm also open to a combined direction—for example, using the generic replay abstraction from #1233 while separately deciding which opaque fields should be persisted—but I think that choice should be explicit.

@Qiyuanqiii — answering your question directly, since it's about #1233's abstraction.

On scope and which PR lands, I have no stake. @lizhengfeng101 and @wu21-web should pick whatever makes the end-to-end review easiest. If that's #1393, I'll close #1233 — I'd rather the Gemini bug were fixed this week than that mine is the PR that fixes it. @Moon-Wrecker already owned the process point and the credit section is more generous than I'd have asked for, so no complaint from me there.

On the abstraction, I do think the generic form is the right default, and there's evidence in this thread rather than just a preference.

extra_content is not a spec field. It is one vendor's name for "metadata you must echo back", and the class of behaviour is not unique to it — the same providers already ship reasoning_content as an unmodelled delta extension, and the SDK's own accumulator carries a // Clamp negative indices to 0 since the API may send -1 for single tool calls comment for a second undocumented deviation. Keying on one literal string means the next vendor that picks a different name is a code change rather than a no-op.

The concrete case is already in @subaru-ye's negative-index payload above. It carries two opaque fields:

{"index":-1,"id":"call_1","type":"function",
 "function":{"name":"file_read","arguments":"{}"},
 "extra_content":{"google":{"thought_signature":"sig"}},
 "provider_flag":true}

A named-key capture replays extra_content and silently drops provider_flag. That is not a hypothetical I constructed — subaru-ye tracked both fields when testing, because both are things a provider could reject the following request over.

The safety property people usually worry about with generic capture is a provider rewriting a tool call's identity. That is handled independently of the key name: reservedToolCallFields blocks id, type and function, so those stay owned by OCR on both the streaming and non-streaming paths. Taking the fields from JSON.ExtraFields rather than from the raw JSON also means the stream's own index framing is excluded for free, which a raw-JSON capture has to strip by hand.

On persistence I think #1393 is right and #1233 is wrong, and I would drop my behaviour. I made it json:"-" because #947 asked for opaque fields to stay out of session logs. @Moon-Wrecker's reasoning in the PR body is better: #1070 landed nativeTurnForJSON after #947 was filed, and it requires the analogous Anthropic thinking signature in the llm_response record so that an exported session can reconstruct a byte-identical request. Excluding the OpenAI-side equivalent makes the two families inconsistent for no benefit. The wire format is unchanged either way.

So the combination you floated is the one I would argue for: generic replay abstraction, persisted. The vendor's field name stays out of the type, and the session export stays reconstructable.

Whatever is decided, I am happy to rebase #1233 onto it or to have the pieces lifted — whichever is less work for the reviewers.

One practical note on the credit section, since a mistyped trailer credits nobody: mine is

Co-authored-by: basil-k-aji-dev <[email protected]>

agree with @basil-k-aji-dev's points on generic replay and persistence. it feels like a solid and future proof approach.

following basil's lead, my trailer is:

Co-authored-by: Ayush Dubey <[email protected]>

thanks everyone for collaborating on this!

@Qiyuanqiii, answering your question from yesterday about which behaviour the project should standardise on. Having worked through it, I agree with @basil-k-aji-dev: capture every unmodelled tool-call field, and persist it. That changes my position on capture.

What changed my mind was testing the risk I'd have raised against generic capture, on the provider this is actually about. I took one real tool call from Vertex (google/gemini-3.8-flash) and replayed it five ways:

Vertex returns only extra_content beyond the standard fields today, and accepts extra fields echoed back. A stricter gateway could still reject an echoed field, but that doesn't happen here, and the reserved-field filter in #1233 already keeps id/type/function owned by OCR. The named key, on the other hand, means the next vendor with a different field name is a code change. So generic capture is the better default.

This also showed that this PR was overclaiming. #947 asks for all unknown fields to be preserved, and this PR keeps only extra_content, so Fixes #947 was wrong. I've changed it to Refs #947 and corrected my comment on the issue.

Which PR carries it is @lizhengfeng101's call. I see two options:

1. fix(llm): preserve opaque tool-call fields across OpenAI-compatible turns #1233 lands the capture and closes fix(llm): preserve opaque tool-call fields across OpenAI-compatible turns #947, and I update this PR on top of it with what fix(llm): preserve opaque tool-call fields across OpenAI-compatible turns #1233 doesn't have: persisting the fields in both JSONL records (basil now agrees with that part), the Opaque "400 Bad Request" error with missing details when using Gemini / OpenAI-compatible endpoints #1042 error body, deep-copying the captured fields, and the Responses API test.
2. I bring fix(llm): preserve opaque tool-call fields across OpenAI-compatible turns #1233's capture into this PR with basil's Co-authored-by, so everything lands in one PR.

I'd prefer the first. The approach is basil's, #1233 was written against #947, and Qiyuanqiii has already found it technically sound. But either works for me, and I won't touch the code until you've picked.

@Qiyuanqiii, your token-accounting point applies to both designs, so I'll deal with it once the route is settled.

On credit: I have addresses from @basil-k-aji-dev, @AyushDubey23 and @subaru-ye. @xiaodu55 and @fkdosilovic, would you like co-author credit as well? If so, please post the address you want used; a trailer with an address GitHub doesn't recognise doesn't credit anyone.

CI on aad0b36 is waiting for workflow approval.

Co-author lines for the squash commit, collected in one place so they can be pasted in at merge time:

Co-authored-by: basil-k-aji-dev <[email protected]>
Co-authored-by: subaru-ye <[email protected]>
Co-authored-by: Ayush Dubey <[email protected]>
Co-authored-by: Tao Xin <[email protected]>

These go at the very end of the squash message, after a blank line. Each is the address its owner posted in this discussion, and I've checked that each one links to the right GitHub account, so none of them will show up as plain text.

lizhengfeng101's own line is added by GitHub automatically, since he authored aa200a5. xiaodu55 and fkdosilovic haven't replied yet; if they do, I'll add their lines here.

@Moon-Wrecker thank you for testing it rather than taking my argument on faith — the five-way replay table is more than I had, and the result that Vertex accepts echoed unknown fields is the part I could not check without an endpoint.

Two corrections to your option-1 list, both in your favour.

Deep-copying is already in #1233, as of 058d0cb this morning. @Qiyuanqiii raised it there: ExtraFields is a map, and both NewToolCallMessage and session.copyMessages copied ToolCall values with a plain slice copy, so every copy shared the one map and the byte slices inside it. CloneToolCalls now copies the map and the values; two tests pin it and both fail against the previous revision. So that item comes off the list of things #1233 does not have.

Persistence I have not changed, and I would rather you carried it than that I bolted it on. Your reasoning in the PR body — #1070 landing nativeTurnForJSON after #947 was filed, so the Anthropic thinking signature has to survive into llm_response for a byte-identical replay — is a better reading of the project's current position than the one I followed, which was #947's original request. It is your finding; it should be your commit.

Option 1 works for me, and so does option 2. I have no stake in which PR carries it: @lizhengfeng101 and @Qiyuanqiii should pick whichever is easier to review end to end, and if that is option 2 then bring the capture across and I will close #1233 without ceremony.

On credit, your three trailers are right and the address for me is the one you have. Worth repeating your own point for @xiaodu55 and @fkdosilovic: the address has to be one GitHub already knows, or the line renders as plain text and credits nobody — I have five commits in another repository that prove it, carrying a trailer whose address resolves to no account at all.

@basil-k-aji-dev thanks, and for both corrections. I'd missed that the deep-copy landed in #1233 this morning.

Given what you've said, I'd like to switch my suggestion to option 2. You're fine with either and would close #1233 if the capture comes across, and @lizhengfeng101 asked earlier to keep this in one PR so it can be reviewed end to end. So the simplest path seems to be: I bring #1233's current commits into this PR by cherry-picking them, so your authorship and subaru-ye's trailer come across unchanged, including CloneToolCalls, and move the persistence code over to ExtraFields. This PR then covers #947, #1042 and #1357 together, and #1233 can close.

@lizhengfeng101, if that works for you I'll go ahead. I'm holding the branch at aad0b36 until you confirm.

@NanaseInori review

6

@NanaseInori review

Awaiting for CI results.

Two housekeeping notes now that this is heading for merge, neither of which needs a reply.

#1233 will close as soon as this lands. It still carries approvals from @Qiyuanqiii and @NanaseInori, so I would rather it not sit there looking like a competing option once this is in. I am not asking for it to be reconsidered — my position from earlier stands: whichever PR is easier to review end to end should land, and @lizhengfeng101 asked for this to be one PR. Say the word after merge and I will close it, or close it yourselves, whichever is less work.

The co-author list needs pasting at squash time. @Moon-Wrecker collected it earlier in this thread and noted it has to go into the squash message by hand. Flagging it only because GitHub's squash box does not carry trailers over from branch commits, and it is an easy thing to lose at the last step. The branch currently has @wu21-web's line on a commit; the others are still only in that comment:

Co-authored-by: basil-k-aji-dev <[email protected]>
Co-authored-by: subaru-ye <[email protected]>
Co-authored-by: Ayush Dubey <[email protected]>
Co-authored-by: Tao Xin <[email protected]>

@lizhengfeng101's own line is added automatically since he authored aa200a5.

Worth it for @subaru-ye in particular — they ran the Windows race-detector verification and the SSE reproduction that found the original streaming gap, across several revisions, without having a PR of their own here.

make test passes on 05a7a2d — 2378 tests across the suite, including the 16 added in this PR.

On CI: the runs on this commit are still sitting at "action_required". Workflow runs from a fork need a maintainer to approve them before they start, so there are no results to wait for yet.

@lizhengfeng101 I am also withdrawing my earlier suggestion to pull #1233's generic capture into this PR. With the approvals on this scope, and @Qiyuanqiii confirming that Refs #947 is the right framing, restarting the review now would cost more than it adds. Nothing else is outstanding from my side.

All review conversations on this PR are resolved now, so nothing is open on my side.

Where 05a7a2d stands: three approvals (wu21-web, Qiyuanqiii, NanaseInori), no unresolved threads, CLA signed, and make test green across the suite. The remaining step is a maintainer approving the workflow runs so CI can start.

Please be patient and wait a while，the maintainer is busy with other commitments today.

Hey @Moon-Wrecker, thanks again for getting this merged and for acknowledging the contributions throughout the discussion.

I noticed that the final merged commit doesn't appear to include the Co-authored-by: Ayush Dubey [email protected] trailer that we had agreed on earlier.

Since my contributions to the streaming capture and error-body fix were included in the final implementation, could you please add the co-author attribution if it's still possible?

Thanks again for making sure the contribution lineage stays properly attributed!