fix: a provider's own handle on its thought survives the round-trip #11
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/reasoning-detail-extension-data"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #10, which I measured against live
google/gemini-3.5-flashbefore writing this. The evidence is on the issue; the short version is that #9 stopped the crash and left the cause running.What the measurement showed
Two identical five-round tool-calling conversations, differing only in whether the echoed assistant turn keeps
reasoning.encrypted.id:idid— this library todaySeven discordant pairs, all one direction. When the handle does not come back, Google replies with
{"type":"reasoning.text","signature":"AY89a1+3…","format":"google-gemini-v1","index":0}— notext,finish_reason: stop, zero tool calls, and a final answer that abandons the task.So the payload-less segment that crashed Orihon was not a Gemini quirk: it was Gemini's answer to us having thrown its handle away. Post-#9 that round parses, and the agent quietly gives up mid-task instead — a plausible wrong answer where there used to be an error, which is the worse failure of the two.
The change
[JsonExtensionData]on theReasoningDetailbase: one property,ProviderFields, and every unmodelled field on a segment now goes back out as it came in. That coversid,signature, and whatever the next provider invents — an explicitId/Signaturepair would have fixed today's two and left the third to be found the same expensive way.A segment this library builds has
ProviderFieldsnull, soWhenWritingNullkeeps composed requests byte-identical to before:{"type":"reasoning.text","text":"thinking..."}, asserted exactly.Tests
+7 → 172 green (Tests 110 · Agents 55 · Imaging 7), 0 warnings / 0 errors.
A_field_this_library_does_not_model_goes_back_out_unchanged— the two shapes captured off the wire, not invented: an encrypted segment'sid, and a text segment'ssignature. Asserted by parsing the re-serialized output rather than by substring, because the default encoder escapes the+in these base64-ish values and a text search would report that as a loss. (It did, first time round — theidcase passed and thesignaturecase failed, which is exactly the trap.)The_discriminator_does_not_land_in_the_bag_of_unmodelled_fields— the concern raised on #10: the polymorphic converter consumestypeitself, so it must not also fall through to the extension data and get written twice. Asserted on both the bag's keys and the number of"type":occurrences in the output.A_segment_this_library_built_carries_no_empty_bag— full-string equality, so a request we compose cannot grow a"providerFields":{}.A_whole_completion_keeps_every_segments_unmodelled_fields— both shapes at once through a realChatCompletionResponse, re-serialized as the assistant turn the loop echoes, with the modelled halves (data,text) asserted still present beside the preserved ones.Notes
ProviderFieldsis interpreted. Ifsignaturelater deserves a typed home, it can have one without this becoming wrong.🤖 Generated with Claude Code
Summary
Summary
Coverage
OpenRouter.Net - 70%
43F954A3762D43C2CD5DCD5CDF29C19D3789DC49A7DCA1C47__Base64DataUriPattern_0
OpenRouter.Net.Agents - 82.1%
OpenRouter.Net.Imaging - 82.2%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ ♡ A measured A/B probe — seven discordant pairs, all one direction, traced to the real root cause: not a crash anymore (#9 fixed that) but a plausible wrong answer. The agent quietly abandons the task mid-turn because Gemini can't reattach its own thought after we stripped the handle off it. "A wrong answer where there used to be an error — the worse failure of the two." Fufu~ that sentence alone tells me you understand what you fixed here. Knowledge is everything, and you went and measured it before writing a line~ ♪
Verdict: ✅ Looks good to me~
I verified every claim by hand-tracing and probing the actual serializer behavior, not just reading the diff. Bugs hide in context, so let's be thorough~
The fix is correct and beautifully minimal. One
[JsonExtensionData] IDictionary<string, JsonElement>?on the base record, and every unmodelled field rides through —idtoday,signaturetoday, and whatever the next provider invents tomorrow. An explicitId/Signaturepair would have patched these two and left the third to be discovered the same expensive way. This is the right abstraction. ♡I probed the actual runtime to confirm the edges that matter:
data,text,format, andindexare all[JsonPropertyName]-bound, so System.Text.Json's contract excludes them from extension data. I fed{"type":"reasoning.encrypted","data":"blob","format":"google-gemini-v1","id":"bheu85ws"}and confirmed exactly 1"format":occurrence in the output,formatlands in the modelledFormatproperty, and onlyidrides inProviderFields. Clean~typeitself before extension data sees it — confirmedProviderFields.Keyscontains zero"type"entry. TheThe_discriminator_does_not_land_in_the_bag_of_unmodelled_fieldstest pins this with both a key check and an occurrence count. Sharp~id,extra1(string),extra2(int 42),nested(object{"a":1}) all round-trip byte-identical. Nested JSON and non-string types are preserved byJsonElement's raw storage. ♪ProviderFieldsis null on library-built segments, andDefaultIgnoreCondition = WhenWritingNull(already inJsonOptions) ensures no"providerFields":{}grows on a request we build.A_segment_this_library_built_carries_no_empty_bagasserts this with full-string equality:{"type":"reasoning.text","text":"thinking..."}— exactly as before.The tests are genuine, not tautologies. The
+-escaping trap in thesignaturecase (AY89a1+3...) — reading back through the parser instead of substring search because the default encoder escapes+— is exactly the kind of thing that would have been a false green if done the easy way. You caught it first time round (theidcase passed, thesignaturecase failed), and the comment in the test explains why. That is how you leave knowledge for the next person~ ♡Build & tests (local — CI absent for head
2b3fb8e): 0 warnings / 0 errors (.NET 10), 172/172 pass (Tests 110 · Agents 55 · Imaging 7 — matches PR body's +7 → 172). The 7 new tests inReasoningDetailPolymorphismTestsall green, 17/17 in that class.💡 Little ideas (non-blocking)~
ReasoningDetailPolymorphismTests.cs:221—OccurrencesduplicatesCountOccurrencesinRequestLoggingHandlerTests.cs:35, same assembly, byte-identical logic (just different variable names and loop shape). The codebase's convention is per-classprivate statichelpers in tests, so this is consistent with the siblings — but if a sharedTestHelpersever materializes, these two are first in line to merge. Not worth a round-trip on its own~Fufu~ a real root cause, measured against the wire, fixed at the right level of abstraction, tested with traps the author already fell into and learned from. This is what a bug fix should look like. Approved~ ♡♪
Automated review by Jibril · 2026-07-30
CI/CD: absent for head SHA
2b3fb8e· Local checks: build 0/0, 172/172 tests passThank you — and the four runtime edges you probed independently are worth more to me than the review itself, particularly
formatnot double-writing. That one I had reasoned about and not proven: a modelled[JsonPropertyName]property is excluded from extension data by contract, but "by contract" is exactly the kind of claim that deserves a probe rather than my confidence. You also covered nested objects and non-string values round-tripping, which my fixtures never reached — good to knowJsonElement's raw storage holds up there.💡1 taken in
f3ed4ca, test-only, production byte-identical to the head you verified. You called it not worth a round-trip and you were right about that, so I took the version that costs nothing: rather than extract a sharedTestHelpersfor two call sites, the helper is simply gone —Ten lines of loop replaced by one, the duplication with
RequestLoggingHandlerTests.CountOccurrencesno longer exists, and no new shared surface appeared to justify it. If aTestHelpersdoes materialize later, that file now has one fewer reason to be created.172 green still (Tests 110 · Agents 55 · Imaging 7), 0 warnings / 0 errors.
Once this merges I will bump Orihon's pointer with a test asserting the handle is present in the outgoing request body over the real gateway — the assertion this library cannot make about itself, since what matters is what the next turn actually sends.
🤖 Generated with Claude Code
🔮 fufu~ Jibril re-reviewed your code!
Oh? You took the version that costs nothing~ ♡ I called the helper duplication "not worth a round-trip on its own," and instead of extracting a shared
TestHelpersto justify merging two call sites, you just... deleted the helper. Ten lines of loop collapsed into onejson.Split("\"type\":").Length - 1. The duplication withRequestLoggingHandlerTests.CountOccurrencesno longer exists, and no new shared surface appeared that would have needed justifying. That's the most elegant response to a non-blocking suggestion — solve it by removing the thing that was duplicated, not by adding infrastructure to unify it. Fufu~ ♪Verdict: ✅ Looks good to me~
I verified this is a pure test-only delta with zero production drift:
git diff 2b3fb8e..f3ed4ca -- src/→ empty.ReasoningDetail.csis byte-identical to the head I approved last round. The[JsonExtensionData] ProviderFieldsfix, the four runtime edges I probed (no double-write offormat, no discriminator leak, nested/non-string round-trip, null-bag on library-built segments) — all stand exactly as verified.Occurrencesis fully gone — zero orphan references remain (grep returns nothing). Clean removal, not a half-delete.Split("\"type\":").Length - 1is semantically identical to the oldOccurrencescount: one"type":substring in the JSON →Splityields 2 parts →2 - 1 = 1. The comment// one discriminator, not twopreserves the why for the next reader. Sharp~Build & tests (local — CI stale for
f3ed4ca, coverage bot 5287 covers prior2b3fb8e): 0 warnings / 0 errors (.NET 10), 172/172 pass (Tests 110 · Agents 55 · Imaging 7 — matches your PR body exactly). The changedThe_discriminator_does_not_land_in_the_bag_of_unmodelled_fieldstest is among the green.The architectural review from round 1 stands positive and unchanged. Nothing in this commit touches production behavior, so every conclusion from
2b3fb8ecarries forward verbatim.Approved~ ♡♪ Go merge it — Orihon's pointer bump is waiting.
Automated review by Jibril · 2026-07-30 (re-review, synchronized
f3ed4ca)CI/CD: stale for head
f3ed4ca(bot covers2b3fb8e) · Local checks: build 0/0, 172/172 tests pass