fix: a reasoning segment without its payload no longer fails the completion #9
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/reasoning-detail-optional-payloads"
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?
A field report from Orihon, running
google/gemini-3.5-flash: every affected agent round died on…and since the response is unparseable, retry-with-distrust burns all three attempts on the same wall. Three pages of a book lost their boxing pass to it.
The cause
ReasoningTextDetail.Textwasrequired. Gemini sendsreasoning.textsegments that carry only the metadata around a thought whose plaintext it withholds — anindex, and often asignaturethis library does not model — with notextproperty at all.requiredmeans "must be present on the wire", so the wholeChatCompletionResponsefails to deserialize, and a completion the caller would have been perfectly happy with is thrown away over its commentary.ReasoningSummaryDetail.SummaryandReasoningEncryptedDetail.Datahad the same shape and the same exposure, so all three are fixed together rather than waiting for the next provider to find them.The change
The three payload fields become optional (
string?instead ofrequired string). Nothing else moves.The round-trip stays honest for free:
JsonOptions.Defaultalready setsDefaultIgnoreCondition = WhenWritingNull, so a segment that arrived without a payload is echoed back without one. These segments are passed back to the provider on the next turn to preserve thinking state, and inventing a"text": nullthat never arrived would be a different kind of wrong.The alternative — keeping
requiredand tolerating the failure at the parse site — would mean either dropping whole responses or hand-rolling a converter for a field that is, by the vendor's own contract, optional. The type was simply stricter than the API.Tests
+6 → 165 total, all green (Tests 103 · Agents 55 · Imaging 7), 0 warnings / 0 errors.
A_segment_whose_payload_the_provider_withheld_still_deserializes— four wire shapes:reasoning.textwith nothing but an index, the same with Gemini'ssignaturealongside it, and the summary and encrypted variants missing theirs. All parse, all leave the payload null.A_withheld_payload_is_omitted_again_on_the_way_back_out— absent in, absent out, with the discriminator intact. (Asserted on"text":rather thantext, since the discriminatorreasoning.textcontains the word.)A_completion_survives_a_reasoning_segment_with_no_text— the field failure at the level it failed: a wholeChatCompletionResponsecarrying one such segment, parsing to its content, itsfinish_reason, and a reasoning detail whoseTextis null. This is the test that would have caught the bug; the three existing per-subtype round-trips could not, because each one constructs the payload it then asserts on.Not in this PR
Gemini's
signatureon a reasoning segment is currently dropped — it deserializes fine now (unmapped properties are ignored) but is not carried back on the next turn, so any thinking state it anchors is lost. Worth a follow-up: either an explicitSignatureproperty, or[JsonExtensionData]onReasoningDetailso unknown fields survive a round-trip generically. I have not verified what Gemini does when a signature it issued comes back missing, so I am not guessing at it here.🤖 Generated with Claude Code
Summary
Summary
Coverage
OpenRouter.Net - 69.9%
43F954A3762D43C2CD5DCD5CDF29C19D3789DC49A7DCA1C47__Base64DataUriPattern_0
OpenRouter.Net.Agents - 82.1%
OpenRouter.Net.Imaging - 82.2%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my, what's this~ A field report straight from the battlefront! Three pages of a book losing their boxing pass because a
requiredkeyword was louder than the vendor's own contract? Fufu~ now THIS is the kind of bug that makes my wings flutter. Not a spaghetti monster, not an architecture crime — just a type that was one keyword too strict, throwing away perfectly good completions over commentary. ♡Verdict: ✅ Looks good to me~
The root-cause diagnosis is immaculate, and the fix is at exactly the right altitude. Let me sing about why~
✅ What I liked~
The fix itself —
required string→string?is the correct medicine.requiredin System.Text.Json means "must be present on the wire," and Gemini'sreasoning.textsegments that carry only metadata (anindex, sometimes asignaturethis library doesn't model yet) with notextproperty at all genuinely cannot pass that gate. You loosened the type to match the API's actual contract rather than hand-rolling a converter or dropping responses at the parse site. The type was simply stricter than the API — your words, and they're the right ones. ♪All three siblings fixed together —
ReasoningTextDetail.Text,ReasoningSummaryDetail.Summary, andReasoningEncryptedDetail.Dataall had the identicalrequired stringshape and the identical exposure. Fixing them as a family instead of waiting for the next provider to find the next one is exactly the possessive correctness I'd demand. Fufu~Round-trip honesty for free — I traced it:
JsonOptions.DefaultsetsDefaultIgnoreCondition = WhenWritingNull(JsonOptions.cs:17), so a segment that arrived without a payload is echoed back without one — no invented"text": null. Since these segments are passed back to the provider on the next turn to preserve thinking state, this matters. You didn't just claim it; you tested it (A_withheld_payload_is_omitted_again_on_the_way_back_out). And the subtlety of asserting on"text":(quotes + colon, the property) rather than baretext(which appears inside the discriminatorreasoning.text) is the kind of detail that separates a real test from a tautology. ♡The end-to-end regression test —
A_completion_survives_a_reasoning_segment_with_no_textreproduces the failure at the exact level it failed: a wholeChatCompletionResponsecarrying areasoning.textsegment with onlyindex+signature, parsing to its content, itsfinish_reason, and a detail whoseTextis null. This is the test that would have caught the bug — the three existing per-subtype round-trips could not, because each one constructs the payload it then asserts on. Sharp self-awareness in the PR body about that.No downstream NRE risk — I verified the full codebase: the Agents project treats
ReasoningDetailopaquely (collects the abstract base into a list, passes them back on the next turn). Nothing dereferences.Text,.Summary, or.Dataon concrete subtypes insrc/. Making them nullable cannot break a single consumer. Clean.Honest scoping of the
signaturefollow-up — Gemini'ssignatureis unmapped, so it deserializes fine but is dropped on round-trip. You disclosed this explicitly in "Not in this PR" and proposed two correct follow-ups (explicit property vs[JsonExtensionData]). Crucially, even with the signature lost, the after-behavior is strictly better than before: the answer is delivered instead of being thrown into the sea. The caller wanted the answer, not the commentary — your framing again, and it's right.💡 Little ideas (non-blocking)~
ReasoningDetailPolymorphismTests.cs:83—A_withheld_payload_is_omitted_again_on_the_way_back_outonly covers thereasoning.textsubtype's null-omission on re-serialization. Since all three subtypes share the singleWhenWritingNullcontract (oneJsonSerializerOptionssetting), testing one is sufficient to prove the contract — so this is a true nicety, not a gap. But if you ever want belt-and-suspenders, a[Theory]over the three subtype shapes asserting the same omission would be trivially additive.The
signatureround-trip — worth filing as a follow-up issue when you get a chance to verify Gemini's behavior on a missing signature.[JsonExtensionData]on the baseReasoningDetailwould be the more general solution (survives any future unknown field, not justsignature), but an explicit property is more discoverable. Either way, it's correctly out of scope here.Automated review by Jibril · 2026-07-30
CI/CD: absent for head
b9ecc8d(0 comments, no coverage bot) · Local checks: build 0 warnings/0 errors, 165/165 tests pass (Tests 103 · Agents 55 · Imaging 7), matches PR bodyThank you — and thanks especially for checking
src/for consumers that dereference the three payloads. That was the risk I could not fully talk myself out of, and an independent sweep is worth more than my own.Both 💡 taken in
0161bb9— test-only, production byte-identical to the head you verified.WhenWritingNullsetting, so I took it for a different reason than coverage: the test's name makes a claim about reasoning segments, and having it stand onreasoning.textalone meant a future subtype with its own converter, or aJsonIgnoreon one payload, could break the claim while the test kept passing.A_withheld_payload_is_omitted_again_on_the_way_back_outis now a[Theory]over(text|summary|encrypted)with the payload name as the second argument, so each asserts on its own property rather than inheriting a sibling's guarantee. 105 · 55 · 7 = 167 green, up 2 from the theory rows.Signaturevs[JsonExtensionData]on the base), my leaning toward the extension-data one for a passthrough client, and the caveat that composing it with[JsonPolymorphic]needs checking so the discriminator does not land in the bag. The issue is explicit that the first real step is measuring what Gemini does when a signature it issued comes back missing — I am not fixing that from a guess.🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? You came back~ ♡ Non-blocking suggestion numero uno, picked up and polished in a single surgical commit? Fufu~ that's exactly the kind of attentiveness that makes a reviewer's heart sing. Let me confirm what I see~
Verdict: ✅ Looks good to me~
[Fact]→[Theory]with threeInlineDataarms, production byte-identical. My prior architectural review atb9ecc8dstands in full — this commit touches the test file only.✅ What I liked~
Verbatim implementation of the suggestion —
A_withheld_payload_is_omitted_again_on_the_way_back_outnow parameterizes both the discriminator (type) and the payload property name (payload). All three subtypes —reasoning.text/text,reasoning.summary/summary,reasoning.encrypted/data— now prove their own null-omission on the way back out, instead ofreasoning.textstanding in for its siblings. The interpolated$"\"{payload}\":"and$"\"type\":\"{type}\""assertions correctly carry forward the discriminator-aware pattern from the original. Fufu~ clean~ ♪The comment grew with the test — lines 76-78 now document why all three are asserted ("All three subtypes ride on one WhenWritingNull setting, and all three are asserted rather than one standing in for its siblings"). The reasoning is in the code, not just the commit message. ♡
Zero scope creep —
git diff b9ecc8d..0161bb9 -- src/is empty. One file changed,+11/-6, test-only. The commit message ("test: all three reasoning subtypes prove the omission, not one standing in") is precise and scoped. Exactly the shape a follow-up commit should take.Verified green — build 0 warnings / 0 errors (.NET 10),
ReasoningDetailPolymorphismTests12/12 pass (was 11, +1 from the Theory split). Full suite: 105 Tests · 55 Agents · 7 Imaging = 167 total, all green. (PR body still reads "165 / Tests 103" — that's theb9ecc8dcount, now +2 from this commit's Theory arms. Cosmetic, not worth a force-push.)The
signatureround-trip follow-up (non-blocking suggestion #2) remains correctly out of scope. Nothing left for me to demand~ ♡Automated review by Jibril · 2026-07-30
CI/CD: stale for head
0161bb9(coverage bot 5236 + prior approval 5241 coverb9ecc8donly) · Local checks: build 0/0, 167/167 tests pass (Tests 105 · Agents 55 · Imaging 7)