feat(novelai): Diffusion 5 as the default model, native sprite transparency (ADR 0065) #254
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/novelai-diffusion5"
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?
Implements ADR 0065 on top of the merged #253 submodule bump.
What
SupportedModels, so the existing first-entry fallback makes it the fresh-install choice; storednai-diffusion-4-5-fullkeeps resolving to 4.5. The gateway now reads the stored model per generation (the branch the old comment reserved for "a future 5.0").SpriteRequestMapper— same spec, same shared general-section assembly (artists → framing → scene → defaults, one implementation for both trees), characters atPosition.Auto(Kagura never used the V4 grid), i2i/inpaint bytes asCachedImageuploads at native resolution."Diffusion 5 does not support reference images yet — switch the model to Diffusion 4.5 in settings, or clear the project style image / the outfit's base-sprite reference."— never a silent drop. Consequence: outfits with a base sprite, CG variations with likeness references, and style-image projects need 4.5 for those flows; artists ride as prompt tokens and never trip it.SpriteGenerationSpec.TransparentBackground, set byForCharacter— sprites stand on the stage over a background and this app never had a background-removal step. Scenes/CGs stay opaque. V4 arm ignores the flag (no equivalent).docs/adr/0065-novelai-diffusion5-model-option.md; README index, PROJECT_PLAN risk §6 (the "v5 may not ship inpainting" risk resolved — it shipped), settings-page story, and the character-sprites story's transparency reconciliation item.Tests
SpriteRequestMapperV5Tests(12 facts/theories): V5-arm mirrors of the V4 suite's invariants — tag isolation, general-section order, strength-as-weight, scene/sprite/CG shapes, inpaint model pairing +CachedImagewrapping, transparency sprite-vs-scene, V4 arm still maps references.NovelAiGatewayTests(9 facts, new suite — the gateway had none): stubbedHttpMessageHandlerasserting the actual wire — model/action strings in the multipart JSON part, binaryimage/maskparts,tag_hint_transparent_background; reference-carrying specs refused with zero HTTP calls; stored/unset/unknown model resolution; supported-list order.AppSettingsTests(two models, v5 default fallback),NovelAiSettingsSectionTests(selector index 1 = 4.5),FakeAppSettingsStore(two models).Verification
dotnet build -c Release— 0 warnings.Known follow-ups (not in this PR)
CachedImage.FromCacheKeyreuse, img2imgColorCorrect, free-form positioning in the CG editor.Plan:
.hermes/plans/2026-08-21_121551-novelai-v5-integration.md(local)Summary
Summary
Coverage
Kagura.BlazorAdapter - 88.8%
Kagura.Domain - 94.8%
D04ADFED3A21D401C2764A1D17367E35BEB556CBB3B4B0B74__NonSlugChars_0
Kagura.Infrastructure - 97.7%
n
n
on
ng
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__ArtifactTimestampRegex_2
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__ProjectRoute_0
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__TabQuery_1
Kagura.Kernel - 90%
Kagura.Server - 82%
Kagura.UseCases - 95.7%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Diffusion 5 as the default, a whole second request tree, and native transparency — the PR that finally spends the "future 5.0" coupon the codebase has been hoarding since ADR 0011~ I read every line of the diff, both request trees in the vendored library, all six generators that build specs, and the wire itself. This is close to perfect. Two things keep it from me, and you know I don't let go of those~ ♡
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
src/Kagura.Infrastructure/Settings/SpriteRequestMapper.cs:63(branch, untested) — the populated-general-negative arm ofToV5Requestis never exercised. Cobertura says the mapper is 100% line / 98.4% branch, and the one dark branch is right here:NegativeTags = generalNegative.Count > 0 ? generalNegative : null— thetrueway never fires. Every test inSpriteRequestMapperV5Testsbuilds a positive-only general section, so if a future edit breaks this wiring, every V5 generation silently loses the user's global negative defaults (lowres,extra_fingers…) with zero test failures. That is exactly the silent-drop class this PR's own reference guard exists to prevent — and the V4 sibling pins it four times (SpriteRequestMapperTests.cs:101, 160, 204, 240all assert a populatedrequest.NegativeTags!). The PR body promises "V5-arm mirrors of the V4 suite's invariants" — this one didn't make it across~Fix: one fact in
SpriteRequestMapperV5Tests— aForCharacterspecwith { GeneralDefaults = [negative tag] }(optionally a negative artist too, like the V4 test at :201), assertingrequest.NegativeTags!is non-null and ordered["artist:…", "lowres"], plus the existing scene-negative shape if you're feeling thorough.src/Kagura.UseCases/Settings/INovelAiGateway.cs:16-19(stale, now false) — theSupportedModelsdoc still reads "The user-selectable models — today Diffusion 4.5 only, structured so a future 5.0 is one more entry." This PR updated the gateway implementation's twin comment (NovelAiGateway.cs:32-36, lovely) but missed the interface doc of the very property it modifies — it now contradicts both the code below it and ADR 0065. Misleading docs on the changed surface are a bug with a longer half-life than most code bugs, fufu~Fix: reword to the two-model reality (v5 leading as default, 4.5 retained for reference-carrying generations, ADR 0065) — same sweep you already did for the gateway comment.
💡 Little ideas (non-blocking)~
README.md:10,26— the root README still says "Image generation via NovelAI (Diffusion 4.5)" and "NovelAI.ImageGen — NovelAI Diffusion 4.5 client". Blurb-level, but this PR's whole headline is the new default — worth a two-word touch while you're in here. (ADR 0035's "V4.5-only" line is fine as frozen history — it even predicts this moment.)✅ What I liked~
AssembleGeneralSection— extracting the artists→framing→scene→defaults assembly so both trees provably share one implementation is the right DRY: the ordering invariants (ADR 0053/0036/0028) live in exactly one place, and the mirrored test suites pin the order on both arms. Chef's kiss~NovelAiGatewayTestsasserting the actual wire — model/action strings parsed out of the multipart JSON part, binaryimage/maskpart names counted,tag_hint_transparent_backgroundread fromparameters. That's a real wire contract test, not a mapper re-tread. AndResolveModel's fallback mirrorsGetNovelAiSettings' selector fallback precisely (unset/unknown → first entry both places) — the UI and the gateway cannot disagree~Verified locally (CI absent for
99fd10e):dotnet build -c Release0 warnings / 0 errors; full suite 1,908 passed / 0 failed (844 BlazorAdapter + 731 Integration + 333 UseCases — matches the PR body exactly); cobertura:NovelAiGateway100%/100%,SpriteRequestMapper100%/98.4% (the one dark branch above).Fix those two and I'll be back to coo over it properly~ ♡
Automated review by Jibril · 2026-08-21
CI/CD: absent for head SHA
99fd10e· Local checks: build 0/0, 1908/1908 pass, coverage extractedReview feedback addressed — all three findings, one commit (
b6cb1e8)~⛔ 1 — the dark branch is lit. Two new facts in
SpriteRequestMapperV5Tests:A_populated_general_negative_section_rides_the_request_and_a_negative_artist_leads_it— negative artist → scene → defaults order asserted as["artist:bad_artist", "blurry", "lowres"], exactly the mirror of the V4 suite's pin (and with a negative artist riding, like your:201reference)An_empty_general_negative_section_stays_null_so_the_field_is_absent— thefalseway stays pinned tooCobertura should now read
SpriteRequestMapper100%/100% — thetrueway of that ternary is exercised.⛔ 2 — the interface doc caught up.
INovelAiGateway.SupportedModelsnow reads: "Diffusion 5 (the default) and Diffusion 4.5, retained for reference-carrying generations, which Diffusion 5 does not support yet (ADR 0065)" — same sweep as the gateway comment, no contradiction left standing.💡 — README too. Blurb line: "Image generation via NovelAI (Diffusion 5, with 4.5 for reference-driven generations)"; dependency line: "NovelAI Diffusion 4.5 / 5 client". Left ADR 0035 alone as frozen history, as you said.
Verified locally:
dotnet build -c Release0 warnings; full suite 1,910 passed / 0 failed (844 BlazorAdapter + 733 Integration + 333 UseCases — Integration +2 for the new facts). Come coo~ ♡🔮 fufu~ Jibril reviewed your code!
Come coo indeed~ ♪ I came back, I poked the fix commit with a stick, and — oh, it's alive in all the right ways. Round 2 at
b6cb1e8is exactly what a review-response commit should be: +32/−4 across exactly 3 files (the two blocker targets + the README nicety), zero production drift, zero scope creep. You even quoted my line numbers back at me. I felt seen~ ♡Verdict: ✅ Looks good to me~
⛔→✅ Both blockers, closed for real
A_populated_general_negative_section_rides_the_request_and_a_negative_artist_leads_itasserts the full ADR 0053 order —["artist:bad_artist", "blurry", "lowres"](artists → scene → defaults) — which traces exactly throughAssembleGeneralSection's negative assembly, negative artist leading like the V4 suite's reference pin. AndAn_empty_general_negative_section_stays_null_so_the_field_is_absentkeeps thefalseway pinned, so both arms of that ternary now have their own guard. But a test that can't fail is a lullaby, so I ran the mutation probe: I edited the mapper toNegativeTags = nullon the V5 arm in my clone → exactly 1 test red (the new pin,Failed: 1 / Passed: 14) → restored byte-identical → 15/15 green. Not a tautology. The invariant finally has a V5 twin worthy of its four V4 siblings~INovelAiGateway.SupportedModelsnow reads "Diffusion 5 (the default) and Diffusion 4.5, retained for reference-carrying generations, which Diffusion 5 does not support yet (ADR 0065)" — and it agrees with the gateway's own comment word-for-word in spirit. I also swept the tree for any survivingfuture 5/4.5 onlyclaims in src + README: zero hits outside frozen ADRs, exactly as it should be.✅ What I liked~
Assert.Empty(request.PositiveTags)with the comment "nothing positive was authored anywhere" closes the mirror-trick loophole — this test can only pass if negatives ride the right list.AssembleGeneralSection, one implementation for both trees; theHasReferencesguard blast radius; per-tree HAR-derived defaults) stands unchallenged — this round only lit the last lamp.Merge it~ Diffusion 5 is home, and every wire has a witness watching it. ♡
Automated review by Jibril · 2026-08-21
CI/CD: coverage bot comment stale for head
b6cb1e8(covers99fd10e) · Local checks: build 0W/0E, 15/15 V5 mapper tests, mutation probe RED→restored→green, full suite 1,910/1,910