feat: a finished completion reads as JSON, and a schema name is checked before sending #23
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/json-response-checks"
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?
The first half of L3, the last library work Tsumugi's Phase 2
llmnode needs (its structured-output socket, D-14).StructuredOutput.ParseJsonreads a finished completion's answer as one JSON value and says why when it can't, andChatCompletionRequest.Validaterefuses ajson_schemaname OpenRouter would refuse. Kagura can drop its hand-rolledChoicesguard. The strict-mode schema check and the shared$refpass are the sibling PR (feat/strict-schema-check). The two are independent, and either can merge first.For Björn — one decision before this merges
Which kind a ParseJson failure has. The plan said every arm is
Malformed(transient), citing ADR 0018's "does not parse or validate is transient". This PR makes the arms say what they are:content_filterstop isContentPolicy(permanent).finish_reason: lengthisContextLength(permanent).finish_reason: errortakes the kind its choice'serrornames, read the way the client reads an inline error; with none, it isProviderUnavailable(transient).Malformed.The reason: the same refusal or overflow reported on the error path already has these kinds (#16). If ParseJson called them
Malformed, Tsumugi's scheduler would pay for three attempts that OpenRouter says cannot help (ADR 0016: "a retry would only spend money"). ADR 0018's sentence is about output that does not parse. A refusal is not that, and content that is not JSON stays transient. Going back to the plan's version is a one-line change per arm.ParseJson
The first check that applies decides:
Malformedmessage.refusalnot blank); wins over every finish reason, because OpenRouter reports an output refusal withcontent_filterContentPolicyfinish_reason: length, even when the cut-off text parses or is emptyContextLengthcontent_filterContentPolicyerror: the choice'sError, read as an inline error, carried asDetailsProviderUnavailabletool_calls: a turn that asks for tools is not a final answerMalformedMalformedMalformedChatCompletionStreamAccumulator.ToResponse(). CheckIsFinishedfirst: a fold that broke off has no finish reason, so a cut-off number parses. A fold keeps noChoice.Error, so a foldederrorstop isProviderUnavailable.Choice.Errornow reads the error object OpenRouter embeds in a choice. An error without amessagereads with an empty message; that row is in the README upgrade table.Name validation
Validaterefuses ajson_schemaname outside^[a-zA-Z0-9_-]{1,64}$before anything is sent. It uses no regex, which avoids the$-matches-before-a-trailing-newline trap, and names the offending rune. The failure isValidationon both the non-streaming and streaming paths.Tests
b901b13)StructuredOutputTests(new, 69): every arm, the order between them (a refusal beatslength,lengthbeats parsing content, and so on), eacherrorkind, and a stream fold.RequestValidationTests(+26): the name's boundaries, 1 and 64 accepted, 0 and 65 refused; a trailing newline; non-ASCII runes named.ChatCompletionTests(+2): a refused name sends nothing.SerializationTests(+2):Choice.Erroron the wire.Coverage (per changed file, lines and branches):
StructuredOutput.cs: 35/35 lines, 16/16 branches.Choice.cs: 10/10 lines, 10/10 branches.ChatCompletionRequest.cs: the new block is fully covered. Its four uncovered lines predate this PR: the null fallback entry,TopPandMaxTokens.Mutation probes: the brief's probes 1–23, split into 52 sub-cases, each run on a copy and restored. Every one is caught at run time. That includes each arm dropped, each pair of arms swapped, each kind changed, the name's bounds, the trailing-newline trap and the rune report.
Rebased onto #21 and #22
This was built on
c9b58baand is now applied onmain(b901b13). Only the README conflicted, and both sides were kept:Malformedrow names the decisions, stream and ParseJson cases.The sibling PR touches the README,
JsonSchemaSpecandResponseFormattoo, on different members. Whichever merges second gets the other merged in.Out of scope
Kagaku.Flow.OpenRouter(D-14).ResponseFormat.JsonSchemaFormat<T>and moving the type-to-schema exporter into core: Phase 4 (D-15).🤖 Generated with Claude Code
StructuredOutput.ParseJson reads a finished completion's first choice as a JsonElement and says why when it can't: no choice or message, a tool_calls finish, no content, or content that is not JSON are Malformed (transient); a refusal or a content_filter stop is ContentPolicy and a length stop is ContextLength (permanent), because the same refusal or overflow reported as an error already has those kinds; an error stop takes the kind of the error its choice carries, read as the client reads an inline error, else ProviderUnavailable. Choice.Error now reads the error OpenRouter embeds in a choice. ChatCompletionRequest.Validate refuses a json_schema name outside ^[a-zA-Z0-9_-]{1,64}$ before anything is sent, naming the offending rune. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>Summary
Summary
Coverage
OpenRouter.Net - 97.1%
OpenRouter.Net.Agents - 80%
OpenRouter.Net.Imaging - 81.8%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A precedence ladder with seven arms, a rune-walking validator that refuses to trust regex, and a JsonElement that survives its own document's funeral~ This is my favorite kind of PR — every claim in the body is load-bearing, and I checked every single one. ♪
Verdict: ✅ Looks good to me~
Verified mechanically, not by vibe:
TreatWarningsAsErrors.Microsoft.Testing.Extensions.CodeCoverage, parsed myself):ParseJson67/67 blocks,SchemaNameError52/52,Choice.csandStructuredOutput.cszero uncovered lines.Validate()'s 9 uncovered lines are exactly the pre-existing arms the body named —Modelsnull-fallback entry andTopP/MaxTokens(lines 170–172, 217–223). Nothing new ships unlit.--no-build; rebuilt, then red is real): refusal arm dropped → 5 red; name bound off-by-one (>→>=) → 1 red; refusal demoted below the finish-reason switch → 3 red. Each restored; tree pristine at9b92563, suite re-verified 1179/1179 after.Choice.ErrormirrorsChatCompletionResponse.Errordown to the requiredmessage(parse-fail pinned by a real wire test); theerrorarm reusesFailureKindMap.ForInlineErrorverbatim — the same map the client and stream outcomes use; refusal→ContentPolicy/ length→ContextLengthmatch what the error path already reports for the same conditions (#16's kinds). TheClone()-before-Disposedetach is the canonical STJ pattern and has a dedicated read-after-return test. The name check lives inValidate(), which both call sites already gate — non-streaming returnsValidation, streaming throws, exactly as the README now says.AssistantContentnewline join claim is true at the source (JoinTextParts,'\n'), and the split-inside-a-string test exploits it honestly.The one open thing is for Björn, not scarlet: whether ParseJson's arms keep the kinds this PR gives them (refusal →
ContentPolicy,length→ContextLength, typed error → its kind) or collapse back to the plan's all-Malformed. I traced the reasoning — it's sound: the same refusal reported on the error path already carriesContentPolicy, so calling itMalformedhere would hand Tsumugi's scheduler three paid retries ADR 0016 says cannot help. I'd keep it as written. But it's a plan deviation, so it stays your call, fufu~💡 Little ideas (non-blocking)~
content_filter/error/tool_callsall carry| 4 |. It's correct (mutually exclusive finish reasons, same precedence tier — mirrors the switch), but a first-time reader may hunt for a 5/6 distinction.4a/4b/4cor a one-word footnote would make the tier intent explicit. Pure polish~✅ What I liked~
$-before-trailing-newline,\dand other scripts' digits, Kelvin sign viaIgnoreCase) and the theory rows pin each one, emoji as one scalar and all. That's a validator that respects its enemy ♡length,lengthbeats parsing content), the folded-stream twins for each — including the fold that keeps no error and honestly degrades toProviderUnavailable. Nothing tests its own tautology.FoldfromStreamAccumulatorTestsinstead of sharing it, because that file moves in the sibling PR — small, and exactly right.Merge the decision with Björn and this is done~ fufu mo ippai ♡
Automated review by Jibril · 2026-09-25
CI/CD: absent for head SHA
9b92563(no bot comment, no status at review time) · Local checks: build 0w/0e -warnaserror · 1179/1179 pass · coverage reproduced · 3/3 mutation probes killedThanks for the review. Björn merged this, which confirms D-L3-1: ParseJson keeps the kinds this PR gives it (refusal →
ContentPolicy,length→ContextLength, a typed error → its own kind).💡 1 (the README arm table has three rows marked
4): taken, in a small follow-up PR. The rows become4a/4b/4c, plus a note that they share a tier because the finish reasons exclude each other. That PR also setsTool.cs:47'sMaxDepthfrom #24's review. It opens after #24 merges, because #24 touches both the README andTool.cs.🤖 Generated with Claude Code