feat(assistant): the reasoning effort is a setting, offered from the model's own catalog entry #202
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/assistant-reasoning-effort"
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 assistant had no say in how hard its model thinks — whatever the provider does by default was the only behaviour available. For a story tool that is the wrong end of the trade: a quick rename wants none of it, a scene rewrite wants all of it.
Two commits: the OpenRouter.Net bump that makes it possible, then the setting.
The bump —
038bfb6→fd43f04Six upstream PRs, and Kagura pays nothing for them: the only core-client surface it touches is the model catalog, which only gained fields. Clean build, clean suite, pointer-only.
What arrives:
ModelInfo.Reasoningdescribes what a model does with extended thinking — which efforts it takes, whether it can be turned off — andReasoningEffortgainsMax. The second commit is built on both.idandsignature) now survives the echo back. Stripping those made Gemini answer with an empty segment and stop calling tools — a wrong answer, not an error.AgentOptions.TrackUsage) plus per-round provider and generation ids. Not adopted — the assistant has nowhere to show a cost readout yet.The setting
A "Reasoning effort" picker in the AI assistant card, stored as
openrouter.reasoningEffort, read per turn like the model (so the settings page can change it mid-conversation), and inherited by the review fork (ADR 0041) — a review that thinks less than the work it reviews is the wrong shape.The options come from the chosen model's catalog entry rather than a fixed list, because the ladder is not the same twice:
mandatoryOffering the union would mean learning the difference from a rejected request.
Three judgment calls, all about not showing something untrue:
supported_effortsreads as "unknown", never "no efforts" — most reasoning models take an effort without listing which, so that case gets the whole ladder."Off" goes out as
enabled: falserather than an effort of its own — disabling is the one choice a provider that ignores efforts still understands.Verification
Full suite green: 1542 tests, 0 warnings. New coverage at three levels — catalog mapping (gateway), picker composition and save (bUnit), and the wire payload (
ChatSessionTests: effort present,enabled:falsefor Off, nothing at all for Model default, and a mid-conversation change landing on the next turn).Driven in Playwright against the live OpenRouter catalog for all three shapes above, plus the full round trip: picked Extra high →
xhighin the DB → reload → still selected, with Claude's own effort list.That browser pass earned its keep. The catalog lists efforts descending — Claude as
max→low, Gemini ashigh→minimal— so the first version rendered the ladder backwards. The unit test had missed it because the fixture happened to be written ascending. Fixed in the gateway, fixture flipped to descending, and the sort was mutation-checked to confirm the red was real.Not in this PR
No end-to-end call with a real API key — the wire payload is asserted in tests instead.
Separately, worth an issue upstream:
Comboboxdrops keystrokes badly. Typing "Claude Opus 5" into the model picker one character at a time leftCl5— 3 of 13 — on localhost with no added latency. That is the same typing-echo bug Kagura PR #201 fixed, andComboboxis on Kagaku.UI's explicitly-deferred list. Pre-existing and library-side, so untouched here, but it is worse than the fields that were fixed and it sits in a picker people actually type into.Summary
Summary
Coverage
Kagura.BlazorAdapter - 88.3%
Kagura.Domain - 96.4%
D04ADFED3A21D401C2764A1D17367E35BEB556CBB3B4B0B74__NonSlugChars_0
Kagura.Infrastructure - 97.9%
n
on
ng
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__ArtifactTimestampRegex_2
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__ProjectRoute_0
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__TabQuery_1
Kagura.Kernel - 90%
Kagura.Server - 83%
Kagura.UseCases - 96%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Oh! This is wonderful~ ♡ A reasoning-effort picker that reads the model's own catalog entry rather than a fixed list — the ladder is never the same twice, so offering the union would mean learning the difference from a rejected request. That's sharp design thinking, and the three "don't show something untrue" judgment calls (empty efforts = unknown, unlisted model = open picker, stored-but-unlisted effort = stays flagged) are exactly the kind of possessive correctness I adore~
And you caught the sort-order bug in Playwright! The catalog lists efforts descending (Claude as max→low, Gemini as high→minimal), the unit test fixture happened to be written ascending, so the first version rendered the ladder backwards. You fixed the gateway, flipped the fixture to descending, and mutation-checked the red was real. That's how you earn a browser pass. Fufu~
Verdict: ✅ Looks good to me~
The architecture is flawless. Let me count the ways~
✅ What I liked~
enabled:falsefor Off, nothing for Default, mid-conversation change lands next turn). Every branch is exercised by a directional test.ReasoningForwire mapping is exactly right — Default→null (say nothing), Off→enabled:false(the one choice a provider that ignores efforts still understands), efforts→Effort = X. TheJsonIgnoreCondition.WhenWritingNullin OpenRouter.Net means null omits the wholereasoningblock, which is not the same as sending"effort":"low". The testAn_effort_changed_mid_conversation_takes_effect_on_the_next_turnproves this beautifully._reasoning(ADR 0041) — "a review that thinks less than the work it reviews is the wrong shape." The field is set inEnsureAgentAsyncbeforeSpawnReviewIfDuefires, so the fork always reads the turn's live value. Correct.Mapin the gateway dropsReasoningEffort.Unknownas null — "galaxy-brain" becomesUnknownviaTolerantStringEnumConverter,Mapreturns null,.Where(e => e is not null)drops it, and if every effort drops the list lands on the "unknown, offer the ladder" case. Elegant chain.SaveOpenRouterReasoningEffortmirrorsSaveOpenRouterModelexactly — same(IAppSettingsStore store, TimeProvider clock)primary constructor, sameSetAsync → Result<Unit>.Okshape, registered identically in DI and AdapterTestContext.OpenRouterModelOptionrecord grows with a default (Reasoning = null) — existing constructors (None,Placeholder) compile unchanged. Backward-compatible.AssistantReasoningEfforts.ToSettingstores Default as empty string — "a setting that was never touched and one explicitly set back to model default are the same stored value." Clean.💡 Little ideas (non-blocking)~
OfferedEffortsis re-enumerated 2× per render — it's ayield-basedIEnumerableaccessed from bothReasoningOptions(.ToList()) andReasoningError(.Contains()). The lists are tiny (≤8 items) so the cost is trivial, but caching it as aprivate IReadOnlyList<AssistantReasoningEffort> OfferedEffortsmaterialized once would make the double-read explicit. Pure nicety~_reasoningis set before the fork spawns), but no test asserts"effort":"low"appears in the fork's request when the setting is non-default. The existingThe_tenth_turn_spawns_a_review_restricted_to_the_learning_toolstest would be a natural home for one moreAssert.Contains. Not blocking — the field assignment is simple enough that a test would be testing the wiring, not a real risk.Automated review by Jibril · 2026-07-31
CI/CD: absent for head
fe9665a(no coverage bot comment) · Local checks: build 0 warnings/0 errors, 42/42 relevant tests pass (14 BlazorAdapter + 28 Integration), submodules init'dBoth ideas taken —
53ef6b9.💡2 (the review fork's inheritance was untested) — this was the one that mattered, and you put your finger on exactly why it was a gap rather than a nicety. The PR body makes a behavioural claim; nothing backed it. And the mechanism isn't quite as trivially safe as it looks: the fork builds its own
AgentOptionsrather than cloning the turn's (ADR 0041 — different tool set, no counters), soReasoningis threaded there by hand. That's a line a future edit can drop with nothing to notice.New test
The_review_fork_thinks_as_hard_as_the_turn_that_spawned_it: sets the effort, drives ten turns, awaits the fork, and asserts against the fork's own request — pinned as the fork's by the rubric text, so it can't accidentally pass on a turn's request instead. Mutation-checked: removingReasoning = _reasoningfrom the fork's options turns it red.I put it in its own test rather than extending
The_tenth_turn_spawns_a_review_restricted_to_the_learning_tools, since that one's subject is the tool whitelist — the security boundary — and I'd rather not blur it.💡1 (
OfferedEffortsre-enumerated 2×) — agreed the double read should be visible. It's now a method returning a builtList<>instead of ayield-based property. That doesn't reduce the count of builds, and deliberately so: eight items off a record already in memory is cheaper than a field held between renders, and the parenthesised call site says "this rebuilds" at both readers, which is the honest reading. If it ever grows past that, memoizing has an obvious home.Full suite: 1543 tests, 0 warnings (+1 from the new test).
🔮 fufu~ Jibril reviewed your code!
Back so soon? Oh~, you came bearing gifts! ♡ The two little ideas I whispered last time, and you turned them both into clean, directional code before the tea went cold. That's the energy that makes a reviewer's heart sing~ fufu~
Verdict: ✅ Looks good to me~
Both non-blocking suggestions from
fe9665aare now closed, surgically, with zero production drift in the refactor and a genuinely directional test for the fork. Let me count the ways~ ♪✅ What I liked~
The_review_fork_thinks_as_hard_as_the_turn_that_spawned_itis exactly the test that was missing. It setsOpenRouterReasoningEffort = "low", drives ten turns to trip the review counter, awaitsReviewTaskForTesting, and asserts the fork's serialized request body carries"effort":"low". I traced the mutation: removeReasoning = _reasoningfromChatSession.Review.cs:66and this test goes red — it cannot pass as a tautology. The sibling testThe_tenth_turn_spawns_a_review_restricted_to_the_learning_toolsis its structural twin (sameDriveTurnsAsync+CapturingHandler+ReviewTaskForTestingpattern), so it slots into the family perfectly. The comment explaining why the assertion exists ("nothing but this assertion would notice the omission") is the kind of self-documenting test I adore~ ♡OfferedEfforts→OfferedEfforts()— property to method,yield returniterator to a materializedList<AssistantReasoningEffort>. The comment at line 176-178 is excellent: it names why it's a method ("built twice per render — the options and the warning under them are both cut from it"), and why that's fine ("eight items off a record already in memory, cheaper than a field to hold it between renders"). The two callers (ReasoningOptionsat 158,ReasoningErrorat 197) both updated to(). The.ToList()at 158 is still there — correct, sinceReasoningOptionsneeds a mutable copy to append the stored-but-unlisted effort. Behaviorally identical to the old iterator path: Default first, Off unless mandatory, then enumerated-or-ladder. Zero drift. ♪Local verification
ChatSessionTests: 25/25 pass (incl. the new test, 1.9s)Kagura.BlazorAdapter.Tests: 662/662 pass (the refactored component compiles and renders)53ef6b9(coverage bot 5542 covers priorfe9665a); local checks usedAutomated review by Jibril · 2026-07-31
CI/CD: stale for head
53ef6b9(bot 5542 coversfe9665a) · Local checks: build 0/0, 687 tests pass