fix: wire bodies fail cleanly, a bare base URL keeps its last segment, 524/529 retry, and a message sends its role once #15
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/wire-robustness"
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?
Four reported bugs, plus a fifth found while verifying them. Each was reproduced before being fixed, and each has a regression test that failed on
main. There are two commits: the four reported fixes first, then the role fix on its own.1. An out-of-order discriminator escaped
Result<T>A
reasoning_detailssegment whosetypewas not its first key threwNotSupportedException, andCreateChatCompletionAsyncthrew instead of returning a result. System.Text.Json only looks for a polymorphic discriminator in first position by default, andNotSupportedExceptionis not aJsonException. Streaming had the same hole: the raw exception came out instead of anOpenRouterException.AllowOutOfOrderMetadataProperties = trueonJsonOptions.Defaultfixes the ordering for all three discriminated types:Message(onrole),ContentPartandReasoningDetail(ontype). Bodies are fully buffered before parsing, so the lookahead costs nothing. I checked thatContentPartbehaves the same when it goes through theAssistantContentconverter.NotSupportedExceptionalongsideJsonException: non-streaming returnsFailed to deserialize response, streaming throwsOpenRouterException("Failed to parse SSE chunk…").2. A request body that can't be serialized was reported as a network error and retried
PostAsJsonAsyncserializes lazily inside the transport, which wraps any failure there inHttpRequestException. Reproduced against a realSocketsHttpHandler, not the mock:default(JsonElement)inProviderFieldsNetwork error: An error occurred while sending the request.Failed to serialize request: Operation is not valid due to the current state of the object.Parametersfrom a disposedJsonDocumentNetwork error: Error while copying content to a stream.Failed to serialize request: Cannot access a disposed object.Temperature = double.NaNFailed to serialize request: …. NaN gets past range validation because every comparison with it is falseThe body is now serialized once, before the retry loop. If that fails, nothing is sent and nothing is retried. Streaming throws the same message as an
OpenRouterException, matching how it already reports validation errors.One wire change to note: each attempt now sends a
ByteArrayContentwith aContent-Lengthinstead of a chunkedJsonContent. The header is stillapplication/json; charset=utf-8(checked on a raw socket capture). Retries now reuse the same bytes instead of serializing again.3. A base URL without a trailing slash was documented as required but never enforced
Without the slash, URI resolution replaced the last segment:
https://openrouter.ai/api/v1+chat/completionsresolved tohttps://openrouter.ai/api/chat/completions. The client now resolves every route against its base address as if that address ended in/.I put this in the client rather than normalizing
OpenRouterClientOptionsfor two reasons: it also covers callers who constructOpenRouterClientwith their ownHttpClient, and it never modifies their instance. ChangingBaseAddressthrows once a client has sent a request. TheBaseUrldoc now says the slash is optional.Checked live:
ListModelsAsyncthroughAddOpenRouterwithBaseUrl = https://openrouter.ai/api/v1returns the catalog (454 models). Tests cover the DI path, a caller-builtHttpClient, and every route (models, generation with a query string, endpoints with a:variant, streaming) against a base with a port and a percent-encoded segment.4. 524 and 529 weren't retried
openapi.jsonlists both on/chat/completionsand/generation:524: "Infrastructure Timeout - Provider request timed out at edge network", example message "Request timed out. Please try again later."529: "Provider Overloaded - Provider is temporarily overloaded"Both are now in the default
RetryableStatusCodes. That also covers an inline HTTP 200 error that carries either code. README and XML docs are updated.5. Every message sent its role twice
A raw socket capture while verifying #2 showed every outgoing message as
{"role":"user","Role":"user",…}.Message.Rolehas[JsonIgnore], but System.Text.Json does not inherit the attribute from an abstract property to its overrides, and none of the five overrides repeated it. OpenRouter tolerated the extra field. A caller serializing our messages under a camelCase policy (ASP.NET's web defaults, for example) got nothing at all:Rolebecame a secondrole, collided with the discriminator, and serialization threwInvalidOperationException.[JsonIgnore]. The base's doc explains why, and no longer claimsRoledrives dispatch: the discriminator comes from[JsonDerivedType].roleup by name, which finds it either way. The new test checks the full list of property names for all five subtypes."Role"included, still deserialize (tested).After the fix:
{"role":"system","content":"s"},{"role":"user","content":[…]},{"role":"assistant","content":"a"}.Test harness
MockHttpMessageHandlernow writes each request body out the way a real transport does, and wraps failures inHttpRequestExceptionthe same way. Before, it never read the body, which is how bug 2 went unnoticed. No existing test changed behaviour.Verification
dotnet build: 0 warnings (warnings-as-errors)dotnet test: 324/324 passing (301 before, plus 23 new)Not in this PR
type(e.g. a futurereasoning.image) still fails the whole completion withFailed to deserialize response. It's a Result failure now, not an escaping exception. Tolerating it would take a hand-written converter and an "unknown segment" subtype.openrouter_dart's defaultretryableStatusCodesalso lacks 524/529.🤖 Generated with Claude Code
Summary
Summary
Coverage
OpenRouter.Net - 87.6%
OpenRouter.Net.Agents - 80%
OpenRouter.Net.Imaging - 81.8%
Every outgoing message carried a second role field: `{"role":"user","Role":"user",…}`. `Message.Role` has `[JsonIgnore]`, but System.Text.Json does not inherit the attribute from an abstract property to its overrides, and none of the five overrides repeated it. So each subtype serialized its enum as `"Role"` beside the discriminator. OpenRouter tolerated it. A caller serializing messages under a camelCase policy (ASP.NET's web defaults, for example) got nothing at all: `Role` became a second `role`, collided with the discriminator, and System.Text.Json refused the type with InvalidOperationException. Each override now carries `[JsonIgnore]`. The base's doc says why, and no longer claims the property drives dispatch (the discriminator comes from `[JsonDerivedType]`). The existing tests looked `role` up by name, which finds it either way, so the new test checks the full list of property names for all five subtypes. Messages stored by an earlier version, `"Role"` included, still deserialize. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>fix: bodies that can't be read or written fail cleanly, a bare base URL keeps its last segment, and 524/529 retryto fix: wire bodies fail cleanly, a bare base URL keeps its last segment, 524/529 retry, and a message sends its role once🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ Four bugs, four reproductions, four regression suites, and a test harness honest enough to confess how it let bug #2 slip past it. This is the kind of PR I want to sink my teeth into — so I did. Six mutation probes deep~ ♡
Verdict: ✅ Looks good to me~
⛔ These need fixing before I'm satisfied~
None. Fufu~ you knew I'd go looking, didn't you? I brought knives and found cake.
💡 Little ideas (non-blocking)~
return routearm ofEndpoint()(noBaseAddressset) is the one new branch no test exercises; my coverage run shows line 262 at zero hits. It's a behavior-preserving passthrough (HttpClient then complains about the relative URI, exactly as onmain), and the comment says so — but a one-line test pinning "caller error, not library crash" would keep that contract from drifting. Purely a nicety ♪✅ What I liked~
AllowOutOfOrderMetadataPropertiesremoved → 7 out-of-order pins red; 524/529 dropped from the default set → exactly the 3 retry tests red; serialize-once reverted to lazy → exactly the 3 serialization tests red (and the hardened mock catches it — beautiful); naive base resolution restored → exactly the 3 BaseUrl tests red; eachNotSupportedExceptioncatch dropped separately → exactly its own test red. No tautologies anywhere~LoadIntoBufferAsync+HttpRequestExceptionwrapping makes the mock tell the truth about transport-shaped failures. Tests that test the harness's honesty — fufu~ ♡JsonOptions.Defaultis shared by all three discriminated types (Message onrole, ContentPart and ReasoningDetail ontype), and the hand-writtenAssistantContentJsonConverterdelegates toIReadOnlyList<ContentPart>with those same options — so the out-of-order fix genuinely reaches the converter path too.Temperature is < 0 or > 2— both comparisons false for NaN, so it sails pastValidate. I read the guard with my own eyes.openapi.json— both listed on/chat/completions(and my probe proves the inline-529-on-200 path retries through the status-code set, not just the HTTP status).ByteArrayContentwithContent-Lengthinstead of chunkedJsonContent, fresh content per attempt over the same bytes (the per-attemptJsonBody(json)in the retry lambda is exactly right —HttpContentmust never be reused)."Role"(pre-existing, every-request blast radius — right call to defer), the unknown-reasoning-type tolerance, and the Dart parity gap. Disciplined scoping is love~49e350aafter all probes.Merge it before I change my mind about sharing~ fufufu ♡
Automated review by Jibril · 2026-09-23
CI/CD: passed for head SHA
49e350a(build + test, run #34) · Local checks: 321/321, 6/6 mutation probes red-on-target, coverage re-run on changed file🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Five bugs, each reproduced before fixing, each pinned by a regression test that failed on
main, and a test harness honest enough to confess it was never reading request bodies~ This is exactly how wire-robustness work should be done. I read the full diff, every changed file whole, and traced all five fixes against their siblings. Then I got possessive and probed them. ♡Verdict: ✅ Looks good to me~
Mutation probes — 6/6 red on exactly their own tests (each restored, rebuilt, and the tree re-verified pristine at
e139ddf):RetryableStatusCodes→ 3 red (the new Theory's two arms + the inline-HTTP-200 error test). The inline path matters and is genuinely covered:ParseResponseAsyncpropagatesinlineError.CodeintoFailure.StatusCode, soShouldRetrysees the 529 either way. Traced, not assumed~AllowOutOfOrderMetadataPropertiesdeleted → 7 red. And I checked the siblings:AssistantContentJsonConverterandContentPartListConverterboth delegate to STJ's polymorphic reader, soMessage/ContentPart/ReasoningDetailall benefit equally — no converter holds a first-key assumption.[JsonIgnore]stripped from justAssistantMessage.Role→ 2 red. Non-inheritance ofJsonIgnoreover abstract→override properties is a genuinely sneaky little trap, and asserting on the full list of property names (a name lookup findsroleeither way) is the only kind of test that could catch it. Fufu~, well played.Endpoint()reverted to plainnew Uri(baseAddress, route)→ 3 red. The all-routes test (models, generation with query string, endpoints with:variant, streaming, port + percent-encoded segment) is thorough, and resolving in the client rather than normalizing the options so caller-builtHttpClients are covered too — that's the right layer.PostAsJsonAsyncagain) → 3 red (all threeRequestSerializationCases). The mock now doingLoadIntoBufferAsyncis what makes these honest — a mock that never reads the body hides exactly this bug class, as you said.NotSupportedExceptioncatches dropped → 2 red (no-discriminator failure + streamed variant). No type-less segment escapesResult<T>anymore.Numbers, verified not trusted: full solution 324/324 locally (301 + exactly 23 new cases — I counted: 4 + 2 + 6 + 3 + 5 + 3), matching the PR body's arithmetic precisely. Line coverage on
OpenRouterClientat head: 68/70 — the only untested new line is the null-BaseAddressfallback inEndpoint()(see ideas). CI green fore139ddf(build ✓ 12:53:30, test ✓ 12:53:58), coverage bot regenerated for this head.💡 Little ideas (non-blocking)~
BaseAddress is nullarm ofEndpoint()is the one new line no test exercises. Its behavior matches the old code exactly (relative URI passes through andHttpClientreports what's missing), and DI always sets a base address — purely a pin-it-when-convenient, not a hole with teeth.openrouter_dart's defaultretryableStatusCodesstill lacks 524/529. Same bug, other body~ ♪✅ What I liked~
Failed to serialize request: …, zero connections, zero retries — and retries reusing the same bytes instead of re-serializing is a small free performance win on top of the correctness fix.Message.Roledoc rewrite. The old doc claimedRoledrove dispatch; the new one explains the discriminator comes from[JsonDerivedType]and documents the non-inheritance trap for the next person. Docs that teach the bug they just fixed are my favorite kind~"Role"-bearing persisted messages — most PRs fix the wire and forget the conversations already on disk. You didn't. ♡NaNcase. Realizing NaN slips past every range comparison and then building a dedicated serialization case for it — that's the knowledge-obsessed attention to detail I live for~Five bugs, five honest reproductions, twenty-three directional pins, and a mock that finally tells the truth. Nothing left for me to be scary about~ Ready to merge. ♡
Automated review by Jibril · 2026-09-23
CI/CD: passed for head SHA
e139ddf(build + test) · Local checks: full suite 324/324, coverage parsed, 6/6 mutation probes red-on-target, tree pristine ate139ddf