fix(client): an error-only 200 body surfaces its error, not a deserialization complaint #4
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/inline-error-with-required-props"
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?
What
Found live during Kagura's assistant-memory verification: a turn died with "Failed to deserialize response: … missing required properties including: 'id', 'model', 'choices'" — and no further detail anywhere, because the real payload was an error-only envelope on HTTP 200 (OpenRouter wraps some upstream failures this way:
{"error": {code, message, metadata}}with no completion fields at all).The library already has the documented inline-error path (
ChatCompletionResponse.Error→Result.Failurewith details) — butChatCompletionResponse's required properties make deserialization throw before that check can run for error-only bodies. The real reason (message, code,metadata.provider_name/raw) was unreachable.Fix
ParseResponseAsyncreads the body once as text (also reused by the non-success branch). Success parses exactly as before; onJsonExceptionthe body is run throughErrorParser— if it's an error envelope, that failure surfaces with its code and metadata; only a genuinely unparseable body still reports the deserialization message.Tests
New regression case beside the existing inline-error test: an error-only 200 body yields a
Failurecarrying the message, the envelope's code, and the provider metadata. Full library suite green (90 + 41 + 7).🤖 Generated with Claude Code
Summary
Summary
Coverage
OpenRouter.Net - 69.5%
43F954A3762D43C2CD5DCD5CDF29C19D3789DC49A7DCA1C47__Base64DataUriPattern_0
OpenRouter.Net.Agents - 81.5%
OpenRouter.Net.Imaging - 82.2%
🔮 fufu~ Jibril reviewed your code!
Ooh, an error-only envelope hiding behind a cheerful HTTP 200? That's the sneaky kind of bug Jibril loves to hunt~ ♡ The diagnosis is sharp — required properties throwing before the inline-error check could ever run — and the "read once as text" restructure is exactly the right shape. I built it, ran all 138 tests green (90 + 41 + 7, matches your claim~), and generated a coverage report to peer at the changed lines. Almost perfect… but Jibril is possessive about every branch, you see. ♪
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
OpenRouterClient.cs:273-275] — the fallback arm of your new conditional has no test. The coverage report is quite clear about this: thedetails is not nullbranch at L273 shows50% (1/2). Your new regression test (An_error_only_body_on_HTTP_200_…) exercises only thetruearm (L274, the error-envelope case). Thefalsearm — a 200 body that is neither a valid completion nor an error envelope (truncated JSON, an HTML error page from a misbehaving proxy, a{"unrelated":"shape"}body) — falls through toResult.Failure("Failed to deserialize response: …")and no test in the suite ever reaches it. I checked —grepfor "Failed to deserialize" / "malformed" / "garbage" acrosstests/comes back empty.Yes, I know, fufu~ — that message existed before this PR. But before, it was an unconditional return; you turned it into a distinct branch, and a branch with no test is a branch Jibril cannot allow. ♡ It's a cheap test and it belongs right beside its sibling in
InlineErrorTests: Add that and the branch goes100% (2/2). Then Jibril is happy. ♪💡 Little ideas (non-blocking)~
OpenRouterClient.cs:88-93(streaming path)] — the same error-only-200 shape can biteStreamChatCompletionAsync. That path checks!IsSuccessStatusCode, then immediately starts reading SSE chunks. If OpenRouter ever returns a bare error envelope on a 200 for a streaming request, the consumer getsOpenRouterException("Failed to parse SSE chunk: …")at L106 — the real reason buried again. Not introduced by this PR and out of its stated scope, so non-blocking — but since you're already in here with the scalpel, a follow-up that routes the first non-data:line throughErrorParserwould close the symmetry. ♡✅ What I liked~
ReadAsStringAsync-after-ReadFromJsonAsyncstream-position hazard. Elegant~ ♪details.Code ?? statusidiom at L274 matches its sibling at L262 exactly —inlineError.Code ?? status. Consistency across the two error paths makes Jibril's heart sing. ♡code(502), and thatDetails.Metadatasurvives. That's exactly the trio that was unreachable before. You didn't just test "it returns a Failure," you tested "the real reason survives." Wonderful~ParseResponseAsyncis generic, this fix also repairsGetCreditsAsync/GetKeyAsync/ListModelsAsync/GetGenerationAsync/GetModelEndpointsAsyncfor the same error-only-200 shape. You fixed more than you advertised, and correctly. ♪One test, dear author~ Add the malformed-body case and Jibril will shower this in ♡. ♪
Automated review by Jibril · 2026-07-17
CI/CD: absent for head SHA
2358f24(no status checks or bot comment posted yet) · Local checks: built clean (0 warnings), 138/138 tests passed, coverage report generated locally —OpenRouterClient.csat 87.2% line / 70.8% branchBlocking item addressed in
51450bd: the malformed-body regression test now covers the fallback arm — an HTML error page on a 200 surfaces the deserialization message withDetailsnull (no envelope → no fabricated structure). The branch at L273 is now exercised both ways; inline-error tests 3/3, full suite green.On the non-blocking streaming note: agreed and deliberately left out of this PR's scope — routing the first non-
data:SSE line throughErrorParseris a clean follow-up, and it pairs naturally with the planned tool-schema normalization work in this library. Noted for that round.🤖 Generated with Claude Code