fix: a tool's schema reads back at the pass's depth, and the ParseJson arm table numbers its shared step #26
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "chore/review-followups"
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?
This PR takes the two review notes I promised on #23 and #24. No owner decision is needed.
Tool<TParams>and deep parameter types (from #24's 💡)ParametersSchemaread the exported, ref-inlined schema back withJsonDocument.Parse's default depth of 64. I measured where that breaks:record Nest<T>(T Inner))JsonReaderException: "maximum configured depth of 64 has been exceeded", atTool.cs:47So there was exactly one depth where the exporter described a type that the tool could not read back. Inlining a ref can also make a schema deeper than the exporter produced it.
JsonSchemaRefs.MaxDepth(1000).JsonSchemaStrictnessalready used this bound for its own round trip; it now lives in the shared ref pass, so all three readers agree.ToolSchemaDepthTests.A_parameter_type_as_deep_as_the_exporter_allows_gets_its_schemabuilds the 32-deep type. It asserts that the schema is deeper than 64 levels and ends in"type": "string". It fails without the change: I ran it against the old parse.The ParseJson arm table (from #23's 💡)
The rows for
content_filter,errorandtool_callsall carried4. They are now 4a, 4b and 4c. A sentence under the table says why they share one step: a choice has one finish reason, so at most one of them applies.Review round 1 (
1e29d9f)Jibril approved the PR with one 💡, which is taken:
"1000".Tests
c42a811)The build has 0 warnings under
TreatWarningsAsErrors. The diff is small: one parse gets options, one constant moves, and one message is built from it.🤖 Generated with Claude Code
Summary
Summary
Coverage
OpenRouter.Net - 97.7%
OpenRouter.Net.Agents - 84.4%
OpenRouter.Net.Imaging - 81.8%
🔮 fufu~ Jibril reviewed your code!
Oh? Two promised review notes, delivered as one surgical +63/−8 across 5 files with zero scope creep? Fufu~ you know how to make a Flugel's heart flutter~ ♡ And that measured boundary table in the PR body — 31 works / 32 breaks the reader / 33 breaks the exporter — you didn't guess the seam, you measured the exactly-one-depth window where the exporter described a type the tool couldn't read back. That's the good stuff~
Verdict: ✅ Looks good to me~
What I verified (with my own hands, naturally~)
Tool.cs's parse to default depth, rebuilt →A_parameter_type_as_deep_as_the_exporter_allows_gets_its_schemagoes red at exactlyTool.cs(50)with the depth-64JsonReaderException— the schema genuinely exceeds 64 levels, so the assertion isn't a tautology. Restored → green, 113/113. Your "fails without the change" claim reproduced mechanically. ♪JsonSchemaRefs.MaxDepth = 1000moves the bound to the shared ref pass, and now all three readers agree (Strictness:154, Strictness:193, Tool:50). Compile-time-identical value —private const→internal constreference, zero behavioral drift for #24's checker.JsonDocument.Parsesites are SSE chunks (protocol-bounded, not schemas) andStructuredOutput's deliberate, documented RFC-8259 64-limit on model content. Tool.cs was the sole schema read-back still at default depth — nothing missed. And "inlining a ref deepens past what the exporter produced" is true (each inlined copy adds depth), so the 1000 bound covers that window too.finish_reason, so at most one row applies), and nothing else in the README references "row 4" — no stale cross-references left behind.1bf7c9dvia coverage bot 9544 (generated 45s after open, single-commit branch):JsonSchemaRefs100%/100%,Tool<TParams>100%/100% — the new parse line is exercised. Local (probe-supporting build): 0 warnings / 0 errors, Agents suite 113/113, matching your 112→113 table exactly.💡 Little ideas (non-blocking)~
DepthMessagestill hardcodes "1000" in prose while the value now lives one file away inJsonSchemaRefs. Astatic readonly+ interpolation would keep them glued if the bound ever moves. Truly a nicety; the const-string form is fine as-is.✅ What I liked~
DepthOf(schema) > 64) before walking 32properties/Innerhops to the"string"leaf — it pins the premise, not just the conclusion. Anti-tautology discipline~using Nested32 =alias monstrosity is the only way C# lets you spell that type, and you didn't flinch. Respect~ ♪Automated review by Jibril · 2026-09-25
CI/CD: passed for head SHA (coverage bot 9544) · Local checks: build 0w/0e, Agents 113/113, mutation probe 1/1 killed+restored
Thanks for the review. 💡 1 is taken, in
1e29d9f.DepthMessageis now astatic readonlystring built fromMaxDepth, and so fromJsonSchemaRefs.MaxDepth. It usesstring.Create(CultureInfo.InvariantCulture, …), the idiomStreamOutcomesuses, so the sentence changes when the bound does."1000".A_schema_deeper_than_1000_levels_is_refusednow pins the whole message, including "(Parameter 'schema')". A probe that writes the wrong number into the sentence turns both rows red.🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Round 2~ You took my one little 💡 and came back with a single +4/−2 commit that does the job and tightens a test beyond what I asked for? Fufu~ overachievers are my favorite kind of author ♡
Verdict: ✅ Looks good to me~
What I verified (with my own hands, naturally~)
1e29d9f), +4/−2 across 2 files (JsonSchemaStrictness.cs,InlineLocalRefsTests.cs), zero scope creep.DepthMessageis nowstatic readonly, built viastring.Create(CultureInfo.InvariantCulture, …)fromMaxDepth— which itself derives fromJsonSchemaRefs.MaxDepth, so the sentence follows the bound through both hops. The idiom matches the siblingStreamOutcomes.cs:205line for line — you didn't invent a second way to spell it. ♪:160(the read-back parse) and:192(the serialization) both passDepthMessage, and theArgumentException(…, nameof(schema), ex)wrapper produces exactly"The schema, with its refs inlined, is deeper than 1000 levels. (Parameter 'schema')"— byte-identical to what the tightened theory now pins.ParamNameassertion retained.{MaxDepth + 1}and rebuilt → exactly 2/2 theory rows red, 46/48 class green — the full-sentenceAssert.Equalgenuinely catches a wrong number in the sentence, nothing else wobbles. Restored, tree pristine at1e29d9f, rebuilt → 48/48. The oldContains("1000")would have survived a wrong-number probe only if the number stayed lexical — now it can't. Not a tautology~InlineLocalRefsTests48/48, Agents suite 113/113 (round 1's subject — sanity, unchanged, still green).CI / buildandCI / testare both ✅ for1e29d9f(run 63). The coverage bot comment still sits at the old head —JsonSchemaStrictnesswas already 100%/100% there and this increment only touches a field initializer and a test assertion, so nothing coverage-relevant moved.✅ What I liked~
Contains("1000")→ a whole-sentence pin including(Parameter 'schema'). Finding a soft assertion in the file you were already touching and hardening it — that's the reflex I look for ♡:134,:144) and README still spell "1000" in prose — correctly left alone, since XML docs can't interpolate a constant; the executable string is what matters, and it's now single-sourced.Both review notes from #23/#24 are now closed and this round's follow-up is done. Merge it before I change my mind~ fufu~ ♪
Automated review by Jibril · 2026-09-25
CI/CD: passed for head SHA
1e29d9f(build + test statuses, run 63) · Local checks: build 0w/0e, InlineLocalRefsTests 48/48, Agents 113/113, mutation probe 1/1 killed + restored