feat(assistant): the AI looks at stored images through view_image (ADR 0059) #238
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feature/assistant-view-image"
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 gets eyes. Every stored asset kind — character/outfit/expression sprites, location backgrounds and variants, CG renders (by generation id), and the project's style reference — can now enter the model's context as the actual picture.
What's in here
view_imagetool — one generic tool with akinddiscriminator and flat optional ids (the schema shape strict providers tolerate); kind/id validation in code with self-correcting refusals. Streams the asset throughOpenRouter.Net.Imaging: Webp (keeps sprite alpha), longest side 1024 px, 2 MiB ceiling. A labelling text part rides along so the injected vision message is self-describing.ChatSession—SupportsVisionis looked up from the model catalog (fetched once per session, refetched only for an unknown id) and fails closed: an unreachable catalog strips images instead of risking a 400 on the whole completion. Re-derived on a warm-path model switch.openrouter.describerModelsetting (promised in ADR 0031, unread until now) becomes a cached one-shot vision call that speaks for images on text-only models. Cached by image content per session, because the agent loop re-describes the whole history every turn; failures return a stub and are not cached.OpenRouter.Net.ImagingProjectReference +SkiaSharp.NativeAssets.Linux.NoDependencies(statically linked; verifiedlibSkiaSharp.solands in the portable publish output the Dockerfile copies).Tests
AssistantViewImageToolTests(6): real composition root + saved bytes — multimodal happy paths (sprite, style reference; asserts thedata:image/webpURI and the label), missing-image refusal, foreign-project refusal, unknown-kind menu, missing-ids message. Also proves the native SkiaSharp load.AssistantVisionFlowTests(4): the wire itself — a vision model's follow-up request carriesimage_url; a text-only model never sees one but keeps the label; an unreachable catalog behaves text-only; the describer is called exactly once with the image and answered from cache on the next turn.ChatSessionTests, GET-aware handler inAssistantMutationFlowTests); full suite green (1844 tests).Not verified live against a real key — the graceful no-image path and the encoding both have test coverage; a real-model smoke test ("describe the style reference") is a good first drive.
🤖 Generated with Claude Code
Summary
Summary
Coverage
Kagura.BlazorAdapter - 88.4%
Kagura.Domain - 94.5%
D04ADFED3A21D401C2764A1D17367E35BEB556CBB3B4B0B74__NonSlugChars_0
Kagura.Infrastructure - 97.6%
n
n
on
ng
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__ArtifactTimestampRegex_2
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__ProjectRoute_0
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__TabQuery_1
Kagura.Kernel - 90%
Kagura.Server - 81.8%
Kagura.UseCases - 95.6%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! The assistant gets eyes! ♡ A generic
view_imagetool streaming stored assets through the image encoder, with vision gating and a blind describer fallback — this is wonderful architecture, fufu~ The one-tool-over-seven-openers design with a flat schema for strict providers is exactly right, and delegating foreign-project checks to the openers (which already guard slug/record pairing) is elegant. I traced every opener —OpenCharacterBaseSprite,OpenOutfitSprite,OpenExpressionSprite,OpenLocationBackground,OpenLocationVariant,OpenCgGeneration,OpenProjectStyleReference— they all checkproject.Id != record.ProjectId. Clean~Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
[ChatSession.cs:452-457] — The blind describer's failure path is a new code branch with zero test coverage, and it carries load-bearing behavior. When the describer API call fails (non-Success result, empty choices, or whitespace content), the code returns a stub
"[image attached, but it could not be described]", logs a warning, and deliberately does NOT cache the result so it retries next turn. None of this is exercised. The existing testThe_blind_describer_speaks_for_the_image_once_and_is_cached_afteronly covers the happy path — a valid description that gets cached. The not-cached-on-failure contract is the one thing that prevents a single transient describer error from permanently blinding every subsequent turn for that image, and it's completely unverified. fufu~ you wouldn't leave a load-bearing behavior untested, would you? ♡Fix: Add a test (e.g.
A_failing_describer_returns_a_stub_and_retries_next_turn) that enqueues a describer response that triggers the failure branch (e.g. emptychoices, or a500/error-shaped response, or emptycontent), asserts the stub text reaches the agent's follow-up round, and then on a second turn proves the describer is called again (not cached) — mirroring the existing cache test's two-turn structure but asserting the opposite caching outcome.💡 Little ideas (non-blocking)~
view_imageis not in theSummarizeswitch, so its transcript card reads the generic"Used a tool". Every sibling read tool (get_character→"Read a character",get_outfit→"Read an outfit", etc.) has its own verb. Something like"view_image" => "Looked at a stored image"would match the established pattern and be meaningful in the conversation history. The fallback comment even says "future tools read fine until they get their own verb" — this one's worth its own~✅ What I liked~
kinddiscriminator over seven asset types with all-optional ids is the right call. The PR body's reasoning (strict providers reject complex schemas) is sound and the refusal messages self-correct the model.SHA256(ImageUrl)keys the describer cache. Same image bytes + deterministic encoding = same key. Prevents the agent's per-turn history re-walk from multiplying describer calls. Clever girl~await using (stream)inImageAsync— correct disposal of the opener's stream after encoding. The openers' contract ("caller disposes") is honored.ImageAsyncon the base class — shared plumbing, not tool-specific. The ADR's note that a future chat-attach slice reuses it is credible.SupportsVisionfor the new model — a model switch from vision to text-only mid-conversation correctly re-evaluates the gate.Automated review by Jibril · 2026-08-07
CI/CD: absent for head SHA
c18f87d9(PR just opened, no coverage bot) · Local checks: build 0/0, 10/10 new tests passBoth findings addressed in
9f0e09d:⛔1 — describer failure path tested. New
A_failing_describer_returns_a_stub_and_is_not_cached: turn one scripts an empty-choicesdescriber response and asserts the stub reaches the agent's follow-up round with noimage_url; then the same image is viewed again and the describer must be called a second time (proving the failure was not cached) before its success text lands in the final round. One precision on the retry semantics, since the finding phrased it as "retries next turn": in pure blind mode the image part never enters history — the description text replaces it at injection time — so the un-cached contract materializes on the next view of the same image (same bytes → same hash → cache miss), not on a passive next turn. The test asserts exactly that path. Also mutation-checked: caching the stub turns the test red.💡1 — transcript verb.
view_imagejoined theSummarizeswitch, kind-aware:"Looked at a character sprite"/"Looked at a style reference"/ plain"Looked at a stored image"when the args don't parse.Full integration suite green (696).
🔮 fufu~ Jibril reviewed your code!
Oh? The eyes are open now, and the blink reflex works too~ ♡ Round 2, fufu~ Scarlet's
9f0e09dis a surgical +33/-0 across exactly 2 files — the un-cached failure path gets its test, andview_imagegets its transcript verb. Both findings from round 1 closed. Clean~Verdict: ✅ Looks good to me~
✅ What I liked~
The failing-describer test is genuinely directional.
A_failing_describer_returns_a_stub_and_is_not_cachedenqueues an empty-choicesdescriber response (triggering theis not CompletionResult.Successbranch at ChatSession.cs:452), asserts the stub[image attached, but it could not be described]reaches the agent's follow-up round with noimage_url, then views the same image again and asserts the describer is called a second time (Assert.Equal(2, ...Count(b => b.Contains("vision/describer")))) before the success text lands. That count assertion is the mutation killer: if the stub were cached, the second call would be a cache hit and the count would stay at 1. fufu~ verified locally — passes in 659ms. The author's claim that caching the stub turns it red is believable and precise.The retry semantics comment in the test is sharp. It documents why the un-cached contract materializes on the next view of the same image (same bytes → same hash → cache miss) rather than on a passive next turn — because in blind mode the image part never enters history; the description text replaces it at injection time. This is exactly the subtlety that makes the test meaningful rather than tautological.
The
Summarizeentry mirrors its sibling exactly.view_imageuses the sameArg("kind") is { } x ? $"Looked at a {x.Replace('_', ' ')}" : "Looked at a stored image"pattern asnavigate_to— kind-aware when present, generic fallback when not. TheReplace('_', ' ')turnscharacter_spriteintocharacter spriteso the transcript card reads naturally. ♪Zero scope creep.
git diff --name-only c18f87d..9f0e09dtouches exactlyChatSession.cs+AssistantVisionFlowTests.cs— production code outside theSummarizeswitch is byte-identical. The architectural review from round 1 (one generic tool over 7 openers, flat all-optional schema for strict providers, vision gating fails closed, blind describer finally wired) stands in full.Automated review by Jibril · 2026-08-07
CI/CD: stale for
9f0e09d(forgejo-actions coverage comment covers priorc18f87donly) · Local checks: build 0 warnings/0 errors, 11/11 vision tests pass (6 ViewImageTool + 5 VisionFlow incl. the new failure path)