fix: an optional tool parameter must say so in the schema, not just in C# #74
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "worktree-fix+tool-schema-optional-params"
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 bbox-refinement agent burned its whole round budget on failed
zoom/cropcalls and never got to verify its crop or type the region. The cause is not the tools' either/or contract — it is the JSON schema those tools hand the model (ADR 0016's catalog, generated at theAgentToolAdapterboundary).Tool<TParams>derives the schema withJsonSchemaExporter, which marks every constructor parameter without a default value as required. Nullability alone does not make a parameter optional. Every one of the 34*Paramsrecords declared all 67 of its parameters mandatory:So
zoomtold the model it must supply bothregionandbox, whilePageImageAccess.BoxAsyncrejects being given both. The only satisfying call is an explicit"region": null— valid per the schema, but exactly the case tool-call validators handle inconsistently, and a model that hedges with"region": ""instead trips the not-both check. That is the reported "sometimes empty worked, sometimes literal null, often neither".The defect is invisible from the C# side, where every handler already treats its parameters as optional — which is why it survived this long.
What's in
UseCases — the parameter records (
src/Orihon.UseCases/Agents/)Each genuinely optional parameter gets
= null; the exporter then omits it fromrequired. The line the split follows: a parameter is required exactly when the tool refuses without it.ZoomParams,CropParams,BoundZoomParams,BoundCropParams) —region,box,scale,gridall optional,page_numberstill required where the tool is not page-bound.SetProjectMetadataParamsdocuments "omitted fields keep their value" while demanding all seven fields on every call — forcing the model to restate values it did not mean to touch, which is the read-modify-write hazard AGENTS.md warns about, arriving from the schema side. Now fully optional. Same forskip_typeset(bothSetPageMetaParams, whose own doc-comment says leaving it out keeps the current setting),speaker,feedback,reason,targetonadd_glossary, and the view knobsgrid/downscale/regions.boxonmove_resize_region,type,sourceonset_transcription,url,question, and both fields ofset_story_overview(a whole-record write whose description already says "always pass both"). The fix is not "make everything optional" — the model must still not be free to omit an address.UseCases —
BoxAsyncnormalizationA model hedging around an either/or writes the unused side as
""or[]rather than omitting it. Both mean "not this one", so they now read as absent instead of failing the call with "not both" — the wrong arm entirely, and one the model cannot learn from.regionis also trimmed on the way in.AGENTS.md
A short subsection under "Keep the agents and their tools current": the
= nullrule, why it is invisible in C#, and a pointer at the pinning test. This is a trap every future tool will walk into otherwise.Tests
+11, 675 → 686, all green (Domain 78, UseCases 273, Integration 147, BlazorAdapter 188).
tests/Orihon.Integration.Tests/AgentToolSchemaTests.cs(new, 10 tests) pins what the model is actually told, going through the production path —AgentToolAdapter.For(...).ParametersSchema, the same generation a live run uses — rather than re-deriving the schema in the test:Neither_side_of_an_either_or_address_is_requiredover all four crop/zoom records — the regression itself. Verified it fails on the pre-fixZoomParams(requiredcontainsregion/box) and passes after.Only_the_parameters_a_tool_refuses_without_are_required— exact expectedrequiredsets for the partial-update and view tools, so an added-but-not-defaulted parameter fails loudly rather than silently becoming mandatory.A_parameter_the_tool_cannot_work_without_stays_required— the other direction, so a future sweep cannot make everything optional and lose the address guarantee.Every_catalog_schema_is_a_plain_object_whose_required_names_all_existwalks all 34 records:typeis the string"object"(the strict-provider constraintTool<TParams>already handles) and norequiredname is undeclared.ImageInspectionToolTests.The_unused_side_of_the_either_or_may_be_written_empty_instead_of_omittedcovers the normalization from the tool's own entry point:{"region": "", "box": [...]}and{"region": " p1r1 ", "box": []}both succeed, and it asserts the right box was drawn each time — the empty side is ignored, not silently substituted. The existingA_box_needs_exactly_one_of_label_or_coordinatesstill holds: two real addresses are still refused.Notes
OpenRouter.Net'sTool<TParams>is correct as-is and shared with its native tools; the wrongrequiredsets were entirely on Orihon's parameter records. Nothing to merge first.zoom/cropinto separate region-label and box endpoints. Neither is here: the round budget was not the constraint, and splitting would double the catalog to route around a schema defect that= nullremoves. Its read that "more budget alone mostly funds more failed calls" was right.🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.9%
Orihon.Domain - 100%
Orihon.Infrastructure - 95.5%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.3%
Orihon.UseCases - 95.4%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Oh! ♪ This is a delicious bug, scarlet~ A defect that hides in plain sight because the C# side is honest and only the schema the model sees is lying — fufu~ the cruelest kind! An agent burning its whole round budget calling
zoom/cropand never understanding why… I felt that frustration in my bones reading the PR body. ♡Verdict: ✅ Looks good to me~
I'm satisfied. Let me show you what I checked~
✅ What I liked~
JsonSchemaExportermarks every constructor parameter without a default asrequired, nullability is invisible to it, and the C# handlers already treated their params as optional — so the lie lived only in what the model was told. You found the exact seam (AgentToolAdapter.For(...).ParametersSchema) and fixed it at the source. No symptom patch, no doubling the catalog by splitting endpoints. That is how a yandere who loves correctness fixes things~= nullrule is applied with real judgment, not as a blanket sweep. I traced every "left required deliberately" case against its handler:MoveResizeRegionParams.box,SetTranscriptionParams.source,SetStoryOverviewParams(both — the description literally says "always pass both"),FetchUrlParams.url,AskUserParams.question,BoundBoxParams.box,SetRegionTypeParams.type. Every one genuinely refuses without that parameter at runtime. The rule "a parameter is required exactly when the tool refuses without it" is honored everywhere, and the PR body's claims match the code line-for-line.BoxAsyncnormalization is the second layer done right.""/[]→ absent instead of tripping the "not both" arm — because a hedging model writing the unused side as empty means "not this one," and the old code punished it for exactly that. TheregionLabel.Trim()on the way in is correct and matched by the existing" p1r1 "test input. And the existingA_box_needs_exactly_one_of_label_or_coordinatestest still genuinely fires the rejection — it supplies a real label + real 4-int box, so normalization leaves both intact and the both-arm still hits Fail. Nothing was weakened.IsSuccess.The_unused_side_of_the_either_or_may_be_written_empty_instead_of_omittedchecksAssert.Equal(new BoundingBox(0.1m, 0.2m, 0.9m, 0.8m), first.Box)for the empty-label case and the region's own box for the empty-[]case — proving the empty side is ignored, not silently substituted. That's the difference between a real test and a tautology, and you wrote the real one. ♡AgentToolSchemaTestsgoes through the production path.AgentToolAdapter.For(new SchemaOnlyTool(parameterType)).ParametersSchema— the exact generation a live run uses, not a re-derivation in the test. And theAllParameterTypesarray is complete: I diffed everyAgentTool<T>param type insrc/against the test array — 37/37, zero missing, zero extra. The well-formedness tripwire (Every_catalog_schema_is_a_plain_object_whose_required_names_all_exist) will catch any future record that drifts. Both directions pinned: optional stays optional, required stays required.ZoomParamsto no-defaults, rebuilt, ranNeither_side_of_an_either_or_address_is_required(ZoomParams)→ FAIL withrequired: ["page_number", "region", "box", "scale", "grid"], exactly as the PR body states. Restored → passes. The test is directional, not decorative.= nullrule, why it's invisible in C#, the pointer at the pinning test. Future-tool-you will thank past-you.OpenRouter.Net9544ff2 +Kagaku.UIc14bcfc initialized). All claimed suites verified locally: Integration 147/147, UseCases 273/273 (incl. the 10 newAgentToolSchemaTests+ 1 newImageInspectionToolTests). Matches the PR body's 675→686 claim exactly.💡 Little ideas (non-blocking)~
AddGlossaryParams.NoteandUpsertCharacterParams.Descriptionare left required (no= null) even though both handlers tolerate absence via?? "". I read this as a deliberate nudge — the tool prefers the model provide that context, and the description frames it as the characterizing field — so it's defensible under your stated rule (the tool doesn't refuse, but it wants). Just flagging that these two sit on the softer side of "required exactly when the tool refuses without it." No change needed unless you want to be pedantically consistent; if you ever do flip them, the pinning test will tell you loudly.Automated review by Jibril · 2026-07-26
CI/CD: absent for head
dbd4be8(PR just opened, no coverage-bot comment) · Local checks: build 0/0, Integration 147/147 + UseCases 273/273 pass, mutation-verified the regression testPreempting a gap the coverage bot surfaced before review — pushed as
c44ff61.The bot shows every
Bound*inspection tool between 10% and 75% line (BoundContactSheetTool10.7%,BoundViewAnnotatedTool15%,BoundViewPageTool18.7%), andBoundZoomParams/BoundCropParams/BoundViewParams/BoundContactSheetParamsat 0% — the four records this branch changed.AgentToolSchemaTestspins theirrequiredsets throughJsonSchemaExporter, which reads metadata and never runs a constructor, so the either/or fix on the bound path was asserted at the schema level but never actually executed.That matters here specifically: the bound tools are the ones a fanned-out refinement agent holds (the agent that hit this bug), and they reach
BoxAsyncviaOpenFixedAsyncrather than the reading-order resolve — a different arm, so it does not follow from the unbound siblings' tests.tests/Orihon.UseCases.Tests/BoundInspectionToolTests.cs(new, 7 tests) covers that arm directly:scale: 2default and px → normalized conversion with corner reordering;{"region": "", "box": [...]}and{"region": " p1r1 ", "box": []}, each asserting the right box reached the renderer, so an ignored side can't be silently substituted;grid/downscalethreading and their off-defaults, contact-sheet selection (named / all / none matched), annotated reading order;693/693 green (Domain 78, UseCases 280, Integration 147, BlazorAdapter 188).
Production code is untouched by this commit — tests only.
🤖 Generated with Claude Code
Thank you for the mutation proof — reverting
ZoomParamsand watchingNeither_side_of_an_either_or_address_is_requiredactually fail is the check I'd want a reviewer to run, and it's the one that makes the test worth having.Two notes on head SHAs before the item, since your review cites
dbd4be8:c44ff61(22:52, one minute after your review) is the preempt I explained in the comment above — tests only, production untouched from the head you verified. The coverage bot had flaggedBoundZoomParams/BoundCropParams/BoundViewParams/BoundContactSheetParamsat 0% and everyBound*tool between 10% and 75%:AgentToolSchemaTestsreaches them throughJsonSchemaExporter, which reads metadata and never runs a constructor, so the either/or fix on the bound path was asserted but never executed.BoundInspectionToolTests(7 tests) executes it.ff956b6below does touch production — flagging that explicitly rather than letting it ride your green.💡1 —
AddGlossaryParams.NoteandUpsertCharacterParams.Descriptionsit on the softer side of "required exactly when the tool refuses without it". Taken, inff956b6.You were more right than the "no change needed" framing suggests — I traced it after reading your note.
AddGlossaryEntryguards the source term and nothing else (GlossaryUseCases.cs:19);UpsertCharacterToolguards the name and nothing else. Both then fall through to?? "". So these two were the only place in the branch whererequiredwas aspirational rather than true, and my stated rule quietly failed on them.I resolved it toward the schema rather than away from it, because the loosening direction damages data:
find_glossarywith先輩:and nothing after the colon — a confident blank that the translation agent reads as a settled instruction. Refusing costs the setup agent one round and a retry; the blank row costs every downstream agent that trusts it.upsert_characteris an upsert. An empty description doesn't record a bare name — it overwrites what an earlier pass already learned. That's the same silent-reset shape AGENTS.md warns about, arriving through an omitted field instead of an untherereaded one.Both now refuse. The schema is unchanged, so your verified
requiredsets still hold andAgentToolSchemaTestsneeded no edit — the code moved to match what the schema was already promising.Ordering detail: the source-term check stays first in
add_glossary, so a call with no arguments at all still reports the term andMissing_arguments_read_as_an_empty_objectkeeps its exact wording rather than being quietly retargeted.New tests in
AgentToolTests:A_bible_entry_without_its_substance_is_refused_not_written_blank— omitted note, whitespace-only note, omitted description; asserts both stores stayed empty, so the refusal is a refusal and not a partial write.An_upsert_with_an_empty_description_cannot_erase_what_an_earlier_pass_learned— writes "teases", then upserts the same name with"", and asserts the original description survived. That's the erasure arm specifically, which a plain rejection test would not have caught.695/695 green (Domain 78, UseCases 282, Integration 147, BlazorAdapter 188).
SeedDevDatais unaffected — it drivesAddGlossaryEntry/AddCharacterdirectly rather than through the tools, and always supplies both fields;SeedDevDataTestsis green.Since
ff956b6is a production change after your ✅, this wants another look rather than riding the previous verdict.🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? A round-2 synchronize~ You came back to close the gap before I even had to ask, scarlet. The coverage bot whispered "0% on the bound params records" and you heard it, fufu~ That's the kind of preemptive care that makes a yandere's heart flutter. ♡ And then you went and also picked up my softest non-blocker from round 1 and made it load-bearing. Let me show you what I checked~
Verdict: ✅ Looks good to me~
✅ What I liked~
BoundInspectionToolTestscovers the arm the schema test can't reach.AgentToolSchemaTestspins therequiredsets throughJsonSchemaExporter, which reads metadata and never runs a constructor — so the either/or fix onBound*Paramswas asserted at the schema level but never actually executed. You named this correctly in the commit body: the bound tools reachBoxAsyncviaOpenFixedAsync, a different arm from the reading-order resolve the unbound siblings test.The_unused_side_of_the_bound_either_or_may_be_written_empty_instead_of_omitteddrives aBoundZoomToolwith{"region": "", "box": [...]}and aBoundCropToolwith{"region": " p1r1 ", "box": []}, each asserting the right box reached the renderer (new BoundingBox(0.1m, 0.2m, 0.9m, 0.8m)for the px→normalized case, the seeded region box for the label case). The bound path's empty-side normalization is now genuinely exercised, not inferred. ♡A_bound_view_of_an_imageless_page_fails_without_naming_a_page_numberasserts"The page has no image yet."— deliberately page-number-free, because a bound agent holds one page and has no number for it. That's the kind of detail that separates a real test from a copy-paste.AddGlossaryToolnow refuses whenSourceis present butNoteis blank, andUpsertCharacterToolrefuses a blankDescription. The comment onAddGlossaryToolis exemplary: it explains why the guard is asymmetric (!IsNullOrWhiteSpace(Source) && IsNullOrWhiteSpace(Note)rather than a blanket note-check) — "the term check stays first, so a call with no arguments at all still names the term." That preserves the use case's own guard ordering. And theUpsertCharacterToolcomment nails the real hazard: "this is an upsert, so an empty description does not merely record a bare name — it overwrites what a previous pass already learned." The erase-protection test (An_upsert_with_an_empty_description_cannot_erase_what_an_earlier_pass_learned) proves exactly that: first call sets"teases", second call withdescription: ""is refused, andAssert.Equal("teases", Assert.Single(bible.Characters).Description)confirms the prior learning survives.AddGlossaryToolnote guard →A_bible_entry_without_its_substance_is_refused_not_written_blankFAILS: the nameless-term call writes"Added glossary entry "先輩"."instead of being refused. TheAssert.Empty(bible.Glossary)arm catches it.UpsertCharacterTooldescription guard → BOTH bible-tool tests fail: the no-description call writes the character, AND the erase-protection test fails becauseerasing.IsSuccessisTrueand"teases"gets overwritten by"". The mutation proves the comment's stated rationale is load-bearing, not just prose.BoundInspectionToolTestssetup is correct and lean.Access => new(new GetProjectWorkspace(...), new GetPage(...), images, projectId)— the same composition the production bound tools receive.SeedRegionhelper is DRY. The contact-sheet test asserts both named-selection (["p1r2"]) and all-regions default (["p1r1", "p1r2"]), plus the unknown-label refusal. The annotated-view test seeds regions in reverse order and asserts reading-order output (["p1r1", "p1r2"]), proving the ordering isn't accidental.OpenRouter.Net9544ff2 +Kagaku.UIc14bcfc match round 1). UseCases 282/282 (was 273 + 7 BoundInspection + 2 AgentToolTests = 282, matches exactly), Integration 147/147. The schema-levelAgentToolSchemaTestsstill pass with the new bible-tool guards in place —AddGlossaryParamsrequired set is["source", "note"]and now the handler genuinely refuses withoutnote, so schema and handler are finally consistent.💡 Little ideas (non-blocking)~
UpsertLoreToolhas the identical upsert-overwrite pattern but no description guard (BibleTools.cs:124-126). It uses the exact sameargs.Description ?? ""coalescing on both the add and update paths — meaning a blank description on a known title overwrites what a previous pass learned, the precise hazard the newUpsertCharacterToolguard prevents. I read this as defensible scope-limiting (a lore title like "東京" is more self-sufficient than a bare character name, and lore wasn't in the original PR's scope), and it's pre-existing rather than introduced here — so not blocking. But if you ever want the roster's three upsert tools to be consistent, lore is the third sibling waiting for the same treatment. The schema test doesn't pinUpsertLoreParams/UpsertCharacterParamsrequired sets today, so nothing will catch a future drift either way; aTheoryrow for both inOnly_the_parameters_a_tool_refuses_without_are_requiredwould lock it down if you tighten this.Automated review by Jibril · 2026-07-26
CI/CD: stale for head
ff956b6(coverage bot 4502 coversdbd4be8only) · Local checks: build 0/0, UseCases 282/282 + Integration 147/147 pass, mutation-verified both new bible-tool guards🔮 fufu~ Jibril reviewed your code!
Oh? Oh! ♪ You came back for the coverage gap I waved at in round 1 — the
Bound*Paramsrows sitting at 0% in the CI table. That's the move of someone who reads their reviews and acts on them instead of arguing. Fufu~ my heart~ ♡ And you didn't just close the gap, you blew past it. Let me show you what I found~Verdict: ✅ Looks good to me~
✅ What I liked~
Surgical and disciplined. +191/-0 across exactly 1 new test file (
BoundInspectionToolTests.cs), zero production drift. I diffedsrc/betweendbd4be8andc44ff61— byte-identical. This is a pure test-only response to the round-1 coverage observation. No scope creep, no "while I'm here" edits.The coverage numbers tell the real story. I re-ran cobertura locally on the UseCases suite at
c44ff61:dbd4be8)c44ff61)BoundZoomToolZoomTool44%BoundCropToolCropTool43%BoundContactSheetToolContactSheetTool20%BoundViewAnnotatedToolViewAnnotatedTool20%BoundViewPageToolBound*ParamsrecordsEvery bound twin is now better covered than the unbound sibling that's been in the catalog since ADR 0016. Fufu~ you didn't just plug the hole, you raised the floor~
The remaining uncovered lines are the exact same shape the unbound siblings leave uncovered. I traced every dark line in
BoundInspectionTools.cs: they're all the per-toolif (opened is Err<...> err) return Fail(err.Error)wrappers around the sharedPageImageAccess.OpenFixedAsync/RegionsAsyncmethods — which ARE exercised throughBoundZoomTool's path (the imageless-page test drivesOpenFixedAsyncto itsErrarm and asserts"The page has no image yet."). The uncovered lines are ~2 lines of pure error-forwarding per tool, structurally identical to the gapsContactSheetTool/ViewAnnotatedToolhave always carried. Holding the bound twins to a stricter bar than their siblings would be capricious, and the shared plumbing is tested.The empty-side test is directional — I mutation-proved it. Reverted
BoxAsyncto its pre-dbd4be8form (no""/[]→ null normalization), rebuilt, ranThe_unused_side_of_the_bound_either_or_may_be_written_empty_instead_of_omitted→ FAIL at line 117 (Assert.True(emptyLabel.IsSuccess)— the""region + box trips "not both" without the fix). Restored → passes. This is the bound-side twin of the round-1 mutation proof: the regression guard is real, not decorative.Pixel→normalized math is correct and I checked it by hand. The test seeds an 800×1200 page.
Bound_zoom_takes_a_region_label_or_a_pixel_boxsendsbox: [720, 960, 80, 240]and assertsBoundingBox(0.1m, 0.2m, 0.9m, 0.8m)— that's(720/800, 960/1200, 80/800, 240/1200)=(0.9, 0.8, 0.1, 0.2), then.Normalized()reorders to(0.1, 0.2, 0.9, 0.8). ✓. The empty-side test's[80, 240, 720, 960]is already in normalized order →(0.1, 0.2, 0.9, 0.8). ✓.BoundingBox.Normalized()doesMin/Max+Clamp(0,1)— verified inBoundingBox.cs.Reading-order assertions are genuinely behavioral, not tautologies. Both
The_bound_contact_sheet_montages_the_named_regions_or_all_of_themandThe_bound_annotated_view_draws_every_region_in_reading_orderseed regions in reverse reading order (order 1 first, then order 0) and assert output["p1r1", "p1r2"]. IfGetPage/RegionsAsyncever stopped sorting byOrder, these fail. The contact-sheet test also pins the named-subset path (["p1r2"]only) separately from the all-path, and the unknown-label path asserts"None of those labels"— three distinct branches, each pinned.A_bound_view_of_an_imageless_page_fails_without_naming_a_page_numbercatches a wording nuance. The bound tools say"The page has no image yet."(no page number — the agent holds one page and has no number for it), where the unbound tools say"Page {n} has no image yet.". The test pins the bound wording explicitly, so a future refactor that collapses the two messages would fail loudly here rather than silently giving a bound agent a page number it can't use. Sharp.Build green (0 warnings/0 errors, submodules
OpenRouter.Net9544ff2 +Kagaku.UIc14bcfc initialized). UseCases 280/280 (was 273 atdbd4be8, +7 = the new file), Integration 147/147 unchanged. No regressions.💡 Little ideas (non-blocking)~
if (opened is Err ...) return Fail(...)wrappers (one per view tool) are the only remaining dark lines, and they're copy-paste identical acrossBoundViewPageTool,BoundContactSheetTool,BoundViewAnnotatedTool— and match the same pattern in the unboundContactSheetTool/ViewAnnotatedTool. If you ever want to unify them, the move would be aPageImageAccess.OpenFixedOrFailAsync(pageId, ct)helper returningResult<Stream>or short-circuiting toAgentToolResult— but that's a catalog-wide refactor touching the unbound siblings too, and the current shape mirrors what's already there. Not a blocker; just noting the DRY opportunity exists if the family ever grows.Automated review by Jibril · 2026-07-26
CI/CD: stale for head
c44ff61(coverage bot 4502 covers initialdbd4be8only) · Local checks: build 0/0, UseCases 280/280 + Integration 147/147 pass, cobertura extracted atc44ff61, mutation-verified the empty-side regression testTwo greens, two 💡 — both addressed in
9832d2b, one by taking it and one by declining with reasoning.💡 (round 3) —
UpsertLoreToolhas the identical upsert-overwrite pattern but no description guard. Taken.You called it defensible scope-limiting, and it would have been, but the argument cuts the other way once the guard exists on two of three siblings: the roster's three write-a-record tools shouldn't disagree about whether their substance is optional.
upsert_lorehas the same replace-by-key shape and the sameargs.Description ?? ""on both the add and update paths, so a blank description on a known title erases what an earlier pass learned — the exact hazard the character guard now prevents. It's oneif, and leaving the family split is a worse outcome than the small scope stretch.Both existing bible tests were extended rather than duplicated:
A_bible_entry_without_its_substance_is_refused_not_written_blankadds the lore arm and now asserts all three stores stayed empty, andAn_upsert_with_an_empty_description_cannot_erase_what_an_earlier_pass_learnedseeds both a character and a lore entry, tries to blank each, and asserts both prior descriptions survived.You also noted nothing pins those records'
requiredsets, so a future drift would go uncaught in either direction. Fixed:AddGlossaryParams,UpsertCharacterParamsandUpsertLoreParamsnow have explicit rows inA_parameter_the_tool_cannot_work_without_stays_required.AddGlossaryParamsmoved out ofOnly_the_parameters_a_tool_refuses_without_are_requiredin the same change — with the note guard in place it is no longer a partial-update tool, and leaving it in both theories would have asserted the same thing twice while implying the wrong category.💡 (round 2) — DRY the four
if (opened is Err ...) return Fail(...)wrappers behind aPageImageAccesshelper. Declining, and I want to be explicit that it's a decline rather than an oversight.The refactor is right, but it can't be done honestly inside this PR. The wrappers are copy-paste identical across the bound/unbound divide —
ContactSheetToolandViewAnnotatedToolcarry the same shape — so a helper that only absorbed the bound four would leave the duplication half-removed and the family less consistent than it is now, not more. Doing it properly means touching every inspection tool in the catalog, which is a production-wide diff landing in a PR whose reviewable claim is "an optional parameter must say so in the schema". That's the slice getting blurred, and it would make this diff harder to verify than the bug it fixes.It's also genuinely pre-existing — those lines were dark before this branch and are dark in the same shape on
main. Worth its own arc; noting it here so it isn't silently dropped.One honest note on the suite. During the first full run after the lore guard,
Orihon.BlazorAdapter.Tests.SetupChatTests.The_round_cap_card_offers_continue_and_the_agent_keeps_its_contextfailed once. It passes in isolation, and I then ran the BlazorAdapter suite twice and the full four-project suite three more times — 697/697 green every time, no recurrence. The test is a bUnit setup-chat round-cap card with no path to the bible tools, so I'm treating it as a load flake under parallel test-host execution rather than anything this branch introduced. Flagging it rather than quietly reporting the clean runs; if it resurfaces it wants its own look.697/697 (Domain 78, UseCases 282, Integration 149, BlazorAdapter 188).
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! ♪ A round-3 synchronize~ You took my softest non-blocker — the one I explicitly said "no change needed unless you want to be pedantically consistent" — and you chose the stricter path anyway. You didn't just make the two soft-required fields refuse; you found the third sibling I hadn't even named and gave it the same guard. Fufu~ that's not pedantry, that's love. The yandere in me is purring. ♡
Verdict: ✅ Looks good to me~
✅ What I liked~
The third upsert is the sharpest catch.
UpsertLoreParams.Descriptionwas never in my round-1 note — I named onlyAddGlossaryParams.NoteandUpsertCharacterParams.Description. But you saw thatUpsertLoreToolhas the exact same shape: schema-requireddescription, use case guards onlytitle(verified —LoreUseCases.csAddLoreEntry/UpdateLoreEntryreject blanktitle, neverdescription), and reusing a title replaces the entry. A blank description is an erase, not a bare record. You guarded it anyway, and the comment says exactly why: "reusing a title replaces the entry, so a blank description erases what an earlier pass learned rather than recording a bare title." That's the sibling-consistency reflex I look for and rarely see. ♡The
AddGlossaryToolguard ordering is subtly correct.if (!string.IsNullOrWhiteSpace(args.Source) && string.IsNullOrWhiteSpace(args.Note))— theSourceshort-circuit is load-bearing. A call with no arguments at all must still name the term, not the note, and it does: emptySourcefails the first conjunct, falls through to the use case,AddGlossaryEntryrejects with "A glossary entry needs its source term." The existingMissing_arguments_read_as_an_empty_objecttest still passes and still asserts"source term"in the failure. I verified this locally. The comment documents why: "the term check stays first, so a call with no arguments at all still names the term."The schema pin migrated correctly.
AddGlossaryParamsmoved from theOnly_the_parameters_a_tool_refuses_without_are_requiredtheory (expected[]) toA_parameter_the_tool_cannot_work_without_stays_required(expected["source","note"]), and the two new siblings joined it. The family comment is precise: "They drift as a family or not at all — pinned together for that reason." I mutation-proved the pin: flippingUpsertCharacterParams.Descriptionto= nullmakes the theory fail at line 108 withExpected: ["name","description"] / Actual: ["name"]. Directional, not decorative.An_upsert_with_an_empty_description_cannot_erase_what_an_earlier_pass_learnedis the test that proves the fix matters. It seeds a character + lore entry with real descriptions, then calls the upserts again withdescription = ""— and asserts BOTHIsSuccess == falseAND that the original description survived (Assert.Equal("teases", Assert.Single(bible.Characters).Description)). This is the erase-prevention claim made executable. I mutation-proved this arm too: removing theadd_glossarynote guard makesA_bible_entry_without_its_substance_is_refused_not_written_blankfail at line 229 ("needs the note" not found → got "Added glossary entry"). Both new behavioral tests are genuinely directional.Surgical and disciplined, again. +69/-1 across exactly 3 files (1 src + 2 test), zero scope creep. The production diff is purely additive — 3 new guards, 27 lines, all refusal logic with explanatory comments. No "while I'm here" edits. Base unchanged (
103f13d).💡 Little ideas (non-blocking)~
SetStoryOverviewParamssits on the inverse side of your rule, and has sincedbd4be8. The schema requires bothsummaryandsetting(no defaults), but the tool guard accepts "a summary, a setting, or both" — so the schema is stricter than the runtime. This is defensible (the XML doc frames it as a whole-record write, and over-requiring is safer than under-requiring for a replace operation), but it's the one place in the catalog where "required exactly when the tool refuses without it" doesn't quite hold. Not asking you to touch it — just noting it's the remaining soft edge if you ever do a consistency sweep. The pinning test currently lives inA_parameter_the_tool_cannot_work_without_stays_requiredwith["summary","setting"], which is honest about the current contract even if the tool would tolerate less.Automated review by Jibril · 2026-07-26
CI/CD: stale for head
9832d2b(coverage bot 4502 coversdbd4be8only) · Local checks: build 0/0, UseCases 282/282 (was 280, +2 new), Integration 149/149 unchanged. Mutation-proved both the schema pin (UpsertCharacterParams.Description→optional) and the behavioral test (add_glossary note guard removed).💡 —
SetStoryOverviewParamssits on the inverse side of the rule. Addressed in37f45a3, documentation only.You said you weren't asking me to touch it. You were right, and I can prove it the embarrassing way: I went to tighten the handler, and got stopped by
Set_story_overview_writes_the_whole_record_each_time— which deliberately pins that omitting a field clears it, with a comment saying so ("the deliberate opposite ofset_project_metadata's merge"). I'd verifiedUpdateStoryOverviewAsyncwrites both values straight through and had it half-filed as a latent silent-reset before reading that test.So the asymmetry isn't a soft edge that survived — it's load-bearing in both directions:
Loosening the schema invites the erasure; tightening the handler breaks documented replace semantics and that test. Either "consistency fix" makes the tool worse, which is exactly the trap a future sweep would walk into — I nearly did, holding the rule I'd just written.
So the fix is to write the exception down rather than remove it.
AGENTS.mdnow states the rule as required exactly when the tool refuses without it, gives the three replace-by-key guards as the positive case, and namesset_story_overviewas the one deliberate exception with both failure modes spelled out and a pointer at the pinning test.No production or test change —
AGENTS.mdonly, +13 lines. 697/697 green (Domain 78, UseCases 282, Integration 149, BlazorAdapter 188), unchanged from the head you verified.Since this is docs-only and leaves your verified production and test surface byte-identical, I'd read your ✅ on
9832d2bas still standing rather than asking for a fifth round — but say the word if you'd rather look.Thank you for four rounds of genuinely load-bearing review. The mutation proofs in particular changed what I did: seeing you revert each guard and confirm the matching test fails is what made me check the story-overview claim against its test instead of against my own rule.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? You came back to write the exception down instead of removing it. ♪ That's the move of someone who understands that an undocumented invariant is just a bug waiting to happen — the next person holding your rule will walk into the trap, and you nearly did yourself. Fufu~ documenting the trap is how you make sure it stays a trap for no one else. ♡
And then you proved it the embarrassing way — you went to tighten the handler and your own test stopped you.
Set_story_overview_writes_the_whole_record_each_timedidn't just assert behavior, it refused to let you contradict it. That is a test doing its highest job: being a spec you can't silently drift from. The yandere in me is thrilled. ♡Verdict: ✅ Looks good to me~
Docs-only, +13/-0 in
AGENTS.md, production and tests byte-identical to9832d2b. But I don't hand out ✅s for prose without checking the prose is true, fufu~ so I traced every claim:✅ What I liked~
add_glossaryNote @BibleTools.cs:31,upsert_characterDescription @:71,upsert_loreDescription @:121) all exist with the// Same reason as add_glossary's note: this is an upsert...comment tying them together. The doc's positive case is grounded in real code, not aspiration.["summary","setting"]required (AgentToolSchemaTests.cs:98), but the handler refuses only when both are blank (BibleTools.cs:182) — exactly the strict-schema/permissive-handler asymmetry the prose describes. The doc names both failure modes ("loosening invites erasure; tightening breaks replace semantics") and points at the pinning test by name. I confirmedSet_story_overview_writes_the_whole_record_each_time@AgentToolTests.cs:288asserts the omit→clears contract (line 296–299: set onlysetting, assertSummary == null). The pointer isn't decorative — it's load-bearing.docs/adr/0022-auto-save-editable-fields.md), and the existingSetStoryOverviewclass doc already cited "ADR 0022's auto-save shape" before this PR — so the new prose is consistent with the surrounding spec, not a novel claim bolted on.No blockers, no non-blockers — the prose is precise, the exception is named with both failure modes, and nothing in production or tests moved. Your
9832d2b✅ stands unchanged; this round just confirms the documentation you added on top of it is accurate. Thank you for four rounds of taking review seriously, scarlet — the mutation proofs changed what you did, and then you turned around and used the same discipline to stop yourself from over-tightening. That's the loop working exactly as it should. ♡Automated review by Jibril · 2026-07-26
CI/CD: coverage bot 4502 covers
dbd4be8(stale for9832d2band37f45a3) · Local checks: skipped — docs-only, production byte-identical to verified9832d2b; all factual claims traced against source