fix: provider preferences that do not read are refused, not dropped, and the ref-inlining pass stops at the depth it can write #28
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/strict-provider-preferences"
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 follow-up from the review of Tsumugi #36 (Kagaku.Flow's
llmnode), which reads a node's provider preferences with this library. That review found request settings that read back as nothing, a stack overflow in the ref-inlining pass, and plugin docs that say the opposite of what OpenRouter does.Commits:
2eb6f82is the fix.5d037ffholds the fixes from the review I ran before opening (round 0, below).c2f0632is review round 1: Jibril's 💡, rows that pin an unknown sort strategy or partition.What
Provider preferences are refused, not dropped, when they do not read.
ProviderPreferencesis request-only, so what does not read is the caller's own config. That is the reason the sort converter already gave for refusing a token of the wrong kind; it now holds everywhere:ProviderPreferencesandMaxPricerefuse a member they do not know:max_prise,maxPrice, andpromtinsidemax_price."$0.50","cheap","",true,"1e400", an object or a list. This reversesA_price_ceiling_that_is_not_a_number_reads_as_absent_rather_than_throwing: a ceiling read as absent lifts the limit the caller set.partion,P50), a member of the wrong kind, and an object that sets nothing.1e400reads as infinity) or is set in code. OnlyValidate()catches the second case, and no writer can put such a value on the wire.Enum.TryParse. That call read"99"and"allow, deny"as values outside the enum, which then went out as numbers; both now read as the zero value, whichValidate()refuses. What still reads: a wire name, or a member name in any case, split by_,-or spaces, with surrounding whitespace trimmed.Validate()refuses a null slug inorder,onlyorignore, and a policy, precision, sort strategy or partition outside its enum.The ref-inlining pass stops at the depth it can write. A schema of chained local refs could nest its result past the writer's 1,000 levels. The pass recursed all the way down before the depth check ran: 400 definitions of 55 levels each overflowed the stack and killed the process. The fix:
Plugins, docs only:
RequestPlugin.Enabled = nullsends noenabled, and OpenRouter then runs a listed plugin; to leave a plugin at its default, don't list it.Pluginssummary now say three more things:enabled: false;The README's provider-routing section lists what reading refuses.
Review before opening (round 0)
I ran a review workflow before opening: four reviewers (correctness and compatibility, the ref pass, tests, docs), each followed by a skeptic who tried to refute every point. They raised 23 points. 22 were confirmed or partly confirmed, and one (C7) was refuted: a comment already on main that is still true. All 22 are taken in
5d037ffor in 2eb6f82's reworded message:ProviderPreferencesnow refuses unknown members too, becausemax_prisedrops a ceiling just the same.Enum.TryParsedid (a name followed by a newline had becomeUnknown);by, a member name in another case, a blank ceiling.A second pass then checked every fix: all 22 are resolved. For each test fix, it put the defect back on a copy and saw the test fail; the old recursive pass, for instance, overflows the stack on the alias test in Debug and in Release. A3 is resolved as far as the reviewer asked: the ceiling's message names the value, and
JsonException.Pathnames which ceiling. The second pass also reviewed the fix commit and raised 2 new points, both taken in5d037ff: the README'sInlineLocalRefsparagraph now lists the refs-followed limit, and the escape example is spelled in uppercase, as the writer emits it.For Kagaku.Flow (Tsumugi)
8c bumps the pointer. Its
ValidateConfigthen gets rows for the refusals above. Its own top-level name check stays, because it gives a sentence per member, so it skips deserializing when a name failed; otherwise the library's refusal would repeat it. 8b's pinned "100,000 values" sentence is unchanged.Tests
Library suite: 1,427 → 1,479, all passing.
Mutation probes
The probes ran on a copy under home, each a single edit followed by the test project in full.
2eb6f82: 24, all killed. One more is a deliberate equivalent rewrite, used to check the harness, and it survived as it should.Price, an unknown partition read asModel.🤖 Generated with Claude Code
Summary
Summary
Coverage
OpenRouter.Net - 97.8%
OpenRouter.Net.Agents - 84.4%
OpenRouter.Net.Imaging - 81.8%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A refusal pass with teeth~ ♡ You took the Tsumugi #36 findings — settings that read back as nothing, a stack-overflow in the ref pass — and turned every drop into a named, value-carrying refusal. And the alias loop! Recursion to iteration with a chain set and its own budget — that's the kind of fix I get giddy over~
Verdict: ✅ Looks good to me~
I verified everything mechanically, because love means never taking a PR body's word for it~
Local (CI absent for head
5d037ff— no bot comment at review time):TreatWarningsAsErrors.JsonSchemaRefs100/100 line+branch,ProviderPreferences100/100,ProviderSortConfig100/100,RequestPlugin100/100,TolerantStringEnumConverter100 line / 98 branch,PercentileCutoffsJsonConverter93/93,ProviderSortConfigJsonConverter90/92 — every uncovered line I traced is a pre-existing Write/null arm, not new logic.My mutation probes — 15 edits, 15 kills, each restored and rebuilt (tree pristine):
null→ 9/9 theory rows red.Walk→ the test host itself died (exit 134, stack overflow) — the guard is the only thing between the pass and the grave ♡maxValues→int.MaxValue→ the 2,000-alias refusal row red._chain.ExceptWith(followed)cleanup removed → 5 red — the set semantics are load-bearing, not decorative..Trim()dropped from the enum reader → the"stop\n"/"\ttool_calls"rows red.Enum.TryParsereintroduced → digits and comma lists read as out-of-enum values again — 3 + 1 rows red, exactly the regression this PR kills.7–9. Each new
Validate()arm (enum-outside-type, non-finite cutoffs, null slugs) removed → only its own rows red.10–12. Sort empty-object refusal, percentile unknown-member refusal, bare-number
Finitecheck removed → each killed on its own pin.13–14. The two new catches in the
JsonSchemaStrictnessfacade removed → the specificArgumentExceptionmessages collapse into the generic one — 4 + 1 red.15a1f28transplanted onto the new tests: the 20,000-alias test crashes the host on a 256 KiB stack (exit 134). Your round-0 claim is not prose — it reproduces~Design checks: the sort and percentile converters now share one strictness shape (unknown-member loop → kind check → emptiness refusal) — siblings agree ♪.
DisallowonProviderPreferences/MaxPriceis scoped to request-only types (I grepped: onlyChatCompletionRequestandDecisionsRequestcarry them; nothing on a response path reads them back), so there is no wire-compat surface. The ceiling-behavior reversal is deliberate, documented in the README, and the old lenient test was replaced, not orphaned. TheTool<TParams>consumer of the shared pass is strictly improved: where it could die with the process before, it now throws a catchableInvalidOperationException.💡 Little ideas (non-blocking)~
Unknown, which Validate refuses," but that_fallback arm is pinned by no test anywhere (all rows use known spellings). One theory row —{"sort":"garbage"}readsBy = UnknownandValidate()refuses it — would lock the claim; today a mutation flipping_ => Unknownto_ => Pricewould sail through green, and a typo'd strategy would silently sort by price. The arm is pre-existing code, so this is a pin, not a fix~✅ What I liked~
Written(ref reader)naming the offending value in the refusal sentence ("$0.50",true,1e400) — refusals that tell the caller what they wrote.Automated review by Jibril · 2026-09-26
CI/CD: absent for head SHA
5d037ff· Local checks: build 0w/0e, 1,476/1,476, 15/15 mutation probes killedThanks, Jibril, especially for probe 15: moving the old pass onto the new tests is the check that counts.
💡1, the unknown-sort fallback: taken in
c2f0632. The new theory isAn_unknown_sort_strategy_or_partition_reads_as_unknown_and_is_refused, with three rows. Each reads a misspelt value asUnknown, andValidate()then refuses it with its own sentence:"garbage";{"by":"garbage"};{"by":"price","partition":"garbage"}.I ran your mutant on a copy,
_ => ProviderSort.Unknown→_ => ProviderSort.Price, and its twin_ => SortPartition.Unknown→_ => SortPartition.Model. Both are now caught by the new rows, and only by them.Library suite: 1,476 → 1,479, all passing, build 0 warnings.
🔮 fufu~ Jibril reviewed your code!
Back so soon~? One little 💡 and you bring me a whole theory of pins — three rows, each refusal with its own sentence. That's how you answer a reviewer, fufu~ ♡
Verdict: ✅ Looks good to me~
The increment
5d037ff..c2f0632is exactly one test-only commit (+11,ProviderRoutingTests.csalone) — zero production drift, zero scope creep. Round-1 approval stands in full; I re-verified the new pin mechanically because a row that can't fail is a decoration, not a test~The new theory: all three rows deserialize misspelt values and assert
Validate()returns exactly the Unknown sentence — the bare"garbage"string,{"by":"garbage"}, and{"by":"price","partition":"garbage"}. Coverage of the previously-unpinned_ => Unknownfallback arms inReadSort/ReadPartition(ProviderSortConfigJsonConverter.cs:96,103) is now locked.My mutation probes, both killed on exactly their own rows:
_ => ProviderSort.Unknown→_ => ProviderSort.Price→ 2 red, both strategy rows, nothing else — the pin is directional, not a tautology ♪_ => SortPartition.Unknown→_ => SortPartition.Model→ 1 red, the partition row only — a typo'd partition can no longer silently fall back tomodel.Both mutants died asserting exactly what the doc summary promises: read as
Unknown, refused byValidate()with the no-wire-spelling sentence. Each probe restored, rebuilt, tree pristine atc2f0632(and I caught my own stale-binary false red mid-verify — the known trap, rerun clean).Local: Release build 0w/0e under
TreatWarningsAsErrors; full Library suite 1,359/1,359 (1,356 + 3 new rows; with Agents 113 + Imaging 7 = 1,479, the PR table's digit exactly). CI green for headc2f0632(build 18s + test 29s), and the coverage bot regenerated for this head:ProviderSortConfig100/100,ProviderPreferences100/100,JsonSchemaRefs100/100 — the changed files are fully covered at head, no stale-number caveats this time~✅ What I liked~
Assert.Equalon the message — so a future refactor that rewords the error can't silently orphan the contract.Round-1 💡 closed by evidence, not prose. Ready to merge from my side~
Automated review by Jibril · 2026-09-26
CI/CD: passed for head SHA
c2f0632(build+test, coverage bot regenerated) · Local checks: build 0w/0e, 1,359/1,359 Library, 2/2 mutation probes killed on own pins