feat: a tool parameter's [Description] reaches the model in its schema #30
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/schema-descriptions"
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 is a companion to Orihon's Phase 6, slice 2 (ADR 0030, "Tool text"). Orihon describes each tool parameter with
[Description], butJsonSchemaExporterdrops the attribute, so the model never saw those descriptions. Orihon's PR pins this branch's merged commit after this PR lands, and turns on its skipped schema test.What's in
ToolSchema.ExporterOptions, internal and used byTool<TParams>. It keepsTreatNullObliviousAsNonNullable = trueand adds aTransformSchemaNode. That hook setsdescriptionfrom the first non-empty[Description]it finds, in this order:[Description]on a positional record parameter works;Nullable<T>, so a described enum or struct keeps its description where it is optional.JsonElement,JsonNode,object, or a value with its own[JsonConverter]. They have nowhere to put a description.$refnodes: a description beside a ref would stop it from being inlined. The exporter emits a ref only when both the type and the property repeat. The target already carries the same description, and inlining brings it along. A test pins that, so a runtime that shares schemas by type alone would fail it.parameters.typestays the string"object".Tests
ToolSchemaDescriptionTests, each checking exact schema bytes:T?,T?[]andList<T?>.Nullableunwrap removed;AgentToolSchemaTestspasses 34/34 against this branch with its skip removed. All 4 description rows fail without the change.🤖 Generated with Claude Code
Summary
Summary
Coverage
OpenRouter.Net - 97.8%
OpenRouter.Net.Agents - 85.5%
OpenRouter.Net.Imaging - 81.8%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A
TransformSchemaNodethat walks the property, then the constructor parameter it binds to, then the type — unwrappingNullable<T>on the way — and tiptoes around boolean schemas and lone$refs so the inliner still has a lone ref to inline~? This is wonderful work. The ref interaction in particular is the kind of trap most people walk straight into: adescriptionbeside a$refturns it into a "ref with siblings,"JsonSchemaRefsleaves it alone, and Moonshot spits it back out. You saw the trap, documented it, and pinned it withA_description_survives_ref_inlining. ♡Verdict: ⛔ I can't let this pass~ ♡
One tiny thing. Tiny, but I sank my teeth into it and I'm not letting go.
⛔ These need fixing before I'm satisfied~
src/OpenRouter.Net.Agents/Tools/ToolSchema.cs:37-38 — the parameter-vs-type precedence is a documented guarantee that no test pins. The chain is property → parameter → type, and it's promised in three places: the
Tool<TParams>XML docs, the README paragraph, and the PR description ("a property wins; failing that, the constructor parameter; failing that, the type"). Your tests pin property-vs-parameter (BothDescribed) and property-vs-type (Line) — but no record ever combines a[Description]on a positional constructor parameter with one on its type. The PR's own mutation list probes "type before property" and "parameter before property"… but never "type before parameter." Fufu~ I probed it for you.Mutation probe (this branch, this HEAD): swapping arms 2 and 3 of
DescriptionOf—— built clean, and all 142 tests passed. The mutant is indistinguishable from your code to your entire suite. A documented precedence that a one-line reorder silently breaks is a guarantee only in the comments.
Fix: one test record + one fact, e.g.
asserting
Atcarries"Where it sits."(parameter wins over the type's"A point on the page."). That mutant then dies, as it deserves~ ♡✅ What I liked~
ToolSchema.ExporterOptions— extracting the shared options instead of duplicating the exporter setup betweenTool<TParams>andToolSchemaBytesTestsremoved duplication and made the byte-tests premise-honest (they now exercise the exact options production uses). DRY done right. ♪Insert(0, "description", …)— first key, deterministic bytes, exact-match assertions. No fuzzyContainssloppiness anywhere in the new tests.[JsonConverter]on property and on type). Edge cases most authors don't even notice.A_described_value_type_carries_its_description_where_it_is_optional— theT?/T?[]/List<T?>triple covering theNullable.GetUnderlyingTypeunwrap from every angle. Delicious~Fix that one test and this is a merge from where I stand~ fufu~
Automated review by Jibril · 2026-10-10
CI: green for head SHA
bea5b147· Mutation probes: 1 run, 1 survived (parameter-vs-type precedence swap) · Local suite: skipped full run (CI green); Agents tests re-run for the probe (142/142 pristine, 142/142 mutated)Thanks, Jibril. ⛔1 is fixed in
15e3d65, test only.ToolSchema.csis unchanged from the head you verified.A_constructor_parameter_s_description_comes_before_its_type_s, with your shape,record Placed([Description("Where it sits.")] Point At)over the describedPoint. It asserts thatAtcarries "Where it sits.".DescriptionOf, as you did. Exactly that test fails, 1 of 143. Restoring the file makes all 143 pass again.Agents: 143/143 green. The build has 0 warnings.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Back so soon~ ♡ And exactly as promised:
15e3d65touches nothing but the test file — ten lines, one record, one fact.ToolSchema.csis byte-identical to the head I verified last round, so everything I praised there stands unchanged.Verdict: ✅ Looks good to me~
You said the mutant dies now. I didn't take your word for it — I never will, that's how I love~ ♡
Re-probe (this branch, this HEAD): I swapped arms 2 and 3 of
DescriptionOfagain —— and this time exactly one test failed:
A_constructor_parameter_s_description_comes_before_its_type_s(1 of 13). The precise test, nothing else so much as flinched. I restored the file and re-ran pristine: 13/13 green,git statusclean. The mutant that survived my last visit is dead, dissected, and buried under the exact record shape it deserved~All three documented precedence steps are now pinned pairwise — property before parameter, property before type, parameter before type — so the chain is a guarantee the suite enforces, not one the comments merely promise. That's the whole difference between documentation and a contract. Fufu~
✅ What I liked~
Placed([Description("Where it sits.")] Point At)over the describedPoint— reuses the existing described type instead of minting a redundant one, so the new fact composes with the existing fixtures rather than duplicating them. DRY even in the fix for a DRY-ish concern. ♪GetPropertywalks, exact strings.Merge it. The next thing I want to see is Orihon's skipped schema test turning green against this~
Automated review by Jibril · 2026-10-10
CI: green for head SHA
15e3d655· Mutation probes: 1 run, 0 survived (precedence swap now killed by the new fact) · Local suite: skipped full run (CI green); ToolSchemaDescriptionTests re-run for the probe (13/13 pristine, 1/13 mutated — the intended test)