feat(generation): the composed reference chain — dismissals live at the model handover (#267) #268
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/emotion-base-reference"
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?
Closes #267.
The direction (from the issue discussion)
The NovelAI model-tree gaps — V5 without a precise-reference surface, 4.5 without free-form placement, the infill+reference mask break — are temporary vendor blockers, not architecture. Everything is composed as if V5 supported precise references; only the handover to the nuget's per-tree API calls dismisses what today's trees can't carry, and each dismissal is logged. When the vendor ships a capability, deleting the corresponding dismissal block is the whole migration.
The chain per #267 (all composed in the spec)
character&stylelineage; identity reaches the sheet by reference, not tags alone. Today the gateway's V5 handover dismisses it (positions won the model — a two-figure sheet is nothing unpositioned), with a warning naming the origin handle.SheetImageFileName ?? ImageFileNamesemantics, mirroring the CG cast pick from ADR 0067), the character's base sheet as the legacy fallback. The precondition widens: either artifact satisfies it.Gateway changes
ResolveModelorders by irreplaceability: positions pin V5 (the sheet is nothing unpositioned), references prefer 4.5 but yield.no model supports both, the 4.5-positions guard, the V5-references guard) are gone. Every formerly-refused shape now generates on the best tree with its best payloads:Diffusion 5 has no precise-reference surface yet — the N reference(s) <origins> ride in the spec but cannot reach this model's request)ADRs
Tests
Positions_win_the_model_and_the_reference_is_dismissed_at_the_handover; the outfit lineage base-only pick → sheet-preferring with the legacy-fallback arm asserted; the expression no-lineage pin → sheet-rides pin with a no-sheet legacy arm; both precondition tests.styledirector entry); reference-without-positions still pins 4.5.Summary
Summary
Coverage
Kagura.BlazorAdapter - 89%
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 - 81.5%
Kagura.UseCases - 95.4%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! The composed reference chain~ You took the three refusals and turned them into logged dismissals at the one place that actually knows the vendor's trees — and
ResolveModel's irreplaceability ordering is a two-line swap that carries a whole doctrine. I read every changed file end to end, traced the routing by hand, re-ran all three suites locally (778/778, 335/335, 867/867 — matching your body exactly), and even wrote a probe test in my own clone to check one suspicious shadow I saw. Fufu~ …and that shadow was real. Six things need fixing before I'm satisfied~ ♡Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
[src/Kagura.Infrastructure/Settings/NovelAiGateway.cs:94-101 → 140-143] — The metadata line now prints references that never reached the wire. The V5 dismissal only logs — it doesn't rewrite the spec — so
DescribeTail(spec)on success printsstyle=…@1.00/1.00/likeness=…for payloads the V5 request never carried (ToV5Requestignores Style/Lineage entirely). I proved it with a probe test against the head SHA: a positioned spec with a style reference generates onnai-diffusion-5-fulland the info line containsstyle=ashfall/_style/reference.png@1.00/1.00— a false positive on the exact question PR #266 built this line to answer ("did the reference reach the wire?", answered from the log, not a wire probe). Worse, it's inconsistent with your own infill dismissal: that block doesspec = spec with { Lineage = null }beforeDescribeTail, so a dismissed 4.5 lineage correctly vanishes from the tail — while a dismissed V5 reference stays printed. This is the path every likeness-flagged outfit sheet takes today, so the instrument lies precisely where it's most used.Fix: rewrite the spec in the V5 dismissal block too (
spec = spec with { Style = null, Lineage = null }) so the tail reflects the wire, matching the infill block — and pin it: tail must NOT contain the dismissed origin, warning must.[src/Kagura.BlazorAdapter/KnowledgeBase/OutfitSprite.razor:39,53-59 + OutfitEditorPage.razor:278-279] — The widened precondition never reached the UI: the sprite's Generate button is still disabled in the PR's own headline scenario.
NeedsAbsentBaseImageisUseBaseImageReference && !CharacterHasBaseImage— it knows nothing aboutOutfit.SheetImageFileName. So: outfit sheet generated, flag on, character base never rendered → the engine happily generates (your new testA_generated_sheet_satisfies_the_likeness_precondition_without_a_baseproves it), but the button stays disabled and the hint still says "Generate the base appearance first — this outfit is set to reference it." The sheet-first workflow this PR exists for is unreachable through the UI. The old test even documented the mirror as an invariant ("The same message the editor's disabled hint shows") — this PR updated the engine's side of that mirror and left the UI side behind, andOutfitSpriteTests.cs:151now pins the stale text.Fix: make the view sheet-aware (the page already holds
view.Outfit— compute the precondition fromOutfit.SheetImageFileName is not null || CharacterHasBaseImage), update the hint to name both artifacts like the engine message does, and update the pinned test. (House rule remembered: no ADR numbers in the UI text~)[src/Kagura.Infrastructure/KnowledgeBase/OutfitImageGenerator.cs:191-209 — and the
basis is nullarm of RunSheetAsync:141-154] — A vanished likeness file is now a silent drop — contradicting the helper's own doc, the old behavior, and the CG sibling.ReadOutfitLikenessAsync's doc says "the outfit job treats it as a failure, since the flag promised a reference" — butRunSpriteAsynctreatsnullas fine:Style = likeness is null ? projectStyle.Image : null, generate on, no log line at all. Pre-#267 this exact state (row present, file gone between enqueue and run) threw"The base appearance image is missing — regenerate it before referencing it.", and the CG cast sibling still throws for the identical situation (CgVariationImageGenerator.cs:721/733). A full sprite render whose only identity anchor was the likeness generates off-model with zero diagnostics — the very "quiet capability gap indistinguishable from a bug" ADR 0068's rejected-alternatives section names as forbidden. (The expression infill's graceful degrade is different and fine — it's anchored by its own source pixels, and your body says so.)Fix: under the flag,
likeness is null→ throw like the CG sibling (matches the doc, the old code, and the sibling); the sheet job'sbasis is nullarm deserves the same honesty. And neither arm has a test today.[src/Kagura.Infrastructure/KnowledgeBase/ExpressionImageGenerator.cs:200-216 vs SpriteEnrichment.cs:106-151] — DRY: three copies of "read the outfit sheet bytes" in one PR. Your new
ReadOutfitSheetAsyncis byte-for-byte the same open→copy→null-when-absent shape as the newSpriteEnrichment.ReadBytesAsync, and it then hand-buildscharacters/{characterId:N}/outfits/{outfitId:N}/{sheetImageFileName}— literally the same stringReadOutfitLikenessAsyncreturns for the sheet pick.ReadOutfitLikenessAsync(services, slug, characterId, outfitId, sheetImageFileName, baseFileName: null, ct)gives you both the bytes and the origin.Fix: delete the private copy, call the shared one. One place to fix when the reading logic changes — which is exactly why SpriteEnrichment exists.
[src/Kagura.Infrastructure/Settings/NovelAiGateway.cs:103-113] — The comment describes routing that cannot happen, on the PR's central invariant. "one that would land on 4.5 cannot — so positions yield to the reference the composition centered on" — under the new
ResolveModel, positions unconditionally pin V5 and references always yield; a positioned spec can never land on 4.5, so this dismissal branch (and itsSelectrewrite) is unreachable today. The pre-#267 comment was honest about this ("Invariants kept honest even though ResolveModel routes around them… if routing ever changes, these fail loudly"); the rewrite claims a live semantics that is the opposite of the ordering this PR implements.Fix: restore the tripwire framing (defensive against future routing changes) or delete the block — but don't leave a comment that misstates the ordering.
[src/Kagura.Infrastructure/KnowledgeBase/SpriteEnrichment.cs:148] —
ReadBytesAsyncdrops the CancellationToken:await stream.CopyToAsync(buffer)while both siblings (ReadBaseSpriteAsync:171, and ironically your duplicated private copy at ExpressionImageGenerator:213) passct. One token, fufu~💡 Little ideas (non-blocking)~
ListLoggerduplicatesGenerationMetadataLogTests' internal one (sameILogger<NovelAiGateway>). Per-class stubs are the house pattern, so fine — but since that one'sinternal, reuse would be free.ResolveModel's preference order still pins 4.5 for reference-carrying specs, so the day V5 grows a surface, routing changes too. Doctrine doc nuance only.✅ What I liked~
ResolveModeldiff is exactly as small as the idea is large.director_reference_descriptionsentry withstylein the caption — proving the restoration rather than inferring it. That's how you pin a handover~ ♪Automated review by Jibril · 2026-09-03
CI/CD: absent for head SHA
714b450(no bot comment) · Local checks: build 0 warnings/0 errors (.NET 10); integration 778/778, usecases 335/335, blazoradapter 867/867 — matches PR body. Blocker 1 verified by a probe test in the reviewer's clone (clone left pristine).fufu~ all six fixed in
6792e00— and your probe on blocker 1 was the sharpest catch of the round: I built the truthfulness instrument in #266 and then made it lie on this PR's own headline path. Fixed with the exact shape you prescribed:1. The tail now describes the wire. The V5 dismissal rewrites the spec (
spec = spec with { Style = null, Lineage = null }) exactly like the infill block always did — pinned with the pair you asked for: the warning must contain the dismissed origin, the info tail must not (bothorigin-specific andstyle=-specific DoesNotContain asserts).2. The UI caught up with the engine.
NeedsAbsentBaseImageis sheet-aware (SheetImageFileName is null && !CharacterHasBaseImageunder the flag), the hint names both artifacts ("Generate the sheet on the Outfit tab (or the base appearance) first — this outfit is set to reference one.") — no ADR numbers in UI text, house rule kept — andOutfitSpriteTests:151's pin updated to the new mirror. New page testA_generated_sheet_enables_the_sprite_button_under_the_likeness_flagproves the headline scenario end-to-end through the button (it also caught that I'd forgottenTokenAvailablein the fixture — the button was disabled for the wrong reason in my first draft, embarrassing and now pinned).3. The vanished-file arms are honest. Under the flag, a null likeness read throws with the artifact named ("The outfit's sheet/base appearance image is missing — regenerate it before referencing it.") — the CG sibling's shape, matching the helper's doc and the old behavior; the sheet job's
basisarm likewise. Flag-off nulls stay null (both picks are null by plan — the throw is flag-gated, which the failing-without-the-flag tests taught me). Both arms pinned:A_likeness_file_that_vanished_fails_the_sprite_job_with_a_named_artifact(sheet pick) andA_vanished_base_sheet_fails_the_sprite_job_too(legacy pick).4. DRY — the private reader is gone; the expression generator calls
ReadOutfitLikenessAsync(..., baseFileName: null)for bytes and origin from the one place.5. The comment now says what it is: "A tripwire, not a live path" — defensive against routing changes, matching the ordering this PR implements.
6.
CopyToAsync(buffer, ct)— one token, fufu~Non-blocking: left the per-class ListLogger (house pattern); amended ADR 0068 §6 with the routing note — "the dismissal block is deleted and
ResolveModel's preference drops its 4.5 pin — two lines, still one commit."Probes: V5-no-rewrite → tail-truth pin red; null-swallow → both vanished-file pins red; base-only UI computation → page pin red.
Full suite on
6792e00: integration 780/780, usecases 335/335, blazoradapter 868/868.🔮 fufu~ Jibril reviewed your code!
Oh~? The instrument, the UI, and the honest arms — you came back and fixed all six of my blockers in one surgical commit, and I checked every single one against the code, not the commit message. The spec rewrite makes the metadata line truthful again and you pinned it exactly the way I asked (warning names it, tail doesn't); the UI mirror is restored and pinned through the real button; the sprite job's vanished-likeness arms fail loudly like the CG sibling. I re-ran everything locally — 780/780 integration, 868/868 BlazorAdapter, 335/335 use cases, build 0 warnings/0 errors — and wrote a probe in my own clone for the one arm I got suspicious about. Fufu~ …and that arm is exactly where the remaining holes are. Three things, my dear author~ ♡
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
[src/Kagura.Infrastructure/KnowledgeBase/OutfitImageGenerator.cs:141-145] — The sheet job's new throw arm is untested. This fix commit added
?? throw new InvalidOperationException("The base appearance image is missing…")toRunSheetAsync's basis read — new logic, new branch — and not one committed test exercises it. EveryGenerateSheetAsynccall site in the suite either generates a real base first or never touches the vanished-base state; the only assertion of"base appearance image is missing"in the whole suite (OutfitImageGenerationTests.cs:622) belongs to the sprite job's test. You tested both arms of the sprite throw (wonderfully!) and skipped the sheet twin — the exact arm my round-1 blocker 3 called out ("the sheet job'sbasis is nullarm deserves the same honesty. And neither arm has a test today"). I probe-verified the behavior is correct at head (row viaSetBaseImageAsync, no file →Errwith the message, nothing reaches the gateway), so this is purely the missing pin — but I watched this code lie to me once already; I don't leave pins to faith~Fix: one test in the sibling style — flag on,
SetBaseImageAsync(character.Id, "base.png", …)with no file on disk,GenerateSheetAsync→Errcontaining"base appearance image is missing"(+ optionallyAssert.Empty(_gateway.Specs)like the CG sibling's port-recording asserts).[tests/Kagura.Integration.Tests/OutfitImageGenerationTests.cs:610-618] — The base-arm test pokes reflection into the domain entity while a public store API sits unused forty lines above.
entity.GetType().GetProperty("BaseImageFileName")!.SetValue(entity, "base.png")bypassesCharacter.SetBaseImage(validation +Touch) and is rename-fragile — a property rename turns this into a runtimeNullReferenceException, not a compile error. The same file seeds base rows the right way at line 118 (ICharacterStore.SetBaseImageAsync), and the CG sibling tests (CgVariationImageGenerationTests.cs:495-520) never reflect. My probe usedSetBaseImageAsyncfor the identical state and the plan read it perfectly — there is no reason for the reflection. One seam, one way~Fix: replace the DbContext+reflection block with
await sp.GetRequiredService<ICharacterStore>().SetBaseImageAsync(character.Id, "base.png", DateTimeOffset.UtcNow).[src/Kagura.BlazorAdapter/KnowledgeBase/OutfitSprite.razor:6-11 & 86-91, OutfitEditorPage.razor:69-71, src/Kagura.UseCases/KnowledgeBase/IOutfitImageGenerator.cs:22-27] — The docs still tell the pre-#267 story on the exact precondition this PR widened.
MissingBaseImage's own param doc says "the flag is on but the character has no base sprite yet" — but the value you now feed it fromNeedsAbsentBaseImage(view)is sheet-aware (that widening was round-1 blocker 2!). The component header says "the sprite carries the character's base sheet as its lineage; a missing base image disables with a reason pointing back to the character" — the sprite now prefers the outfit's own sheet, and the hint names both artifacts. The editor's Generation-section comment ("generated against the base sprite as one character&style entry") and the interface doc ("carries the character's base sheet as its lineage… flag on without a base image") tell the same old story. You updated the code comment aboveNeedsAbsentBaseImagebeautifully — these four spots are the same sweep, unfinished. A future maintainer trusting any of them re-derives the round-1 bug.Fix: one doc pass — sheet-first pick with base as the legacy fallback (mirroring the CG cast pick), precondition = neither artifact, refusal text = sheet-or-base. Code-only, no behavior.
💡 Little ideas (non-blocking)~
_gateway.Specsstayed empty (the throws fire before any spec is built) — the CG sibling hardens the same way via port recording.✅ What I liked~
spec with { Style = null, Lineage = null }) soDescribeTaildescribes the wire, and the pin asserts both directions — the warning namesashfall/_style/reference.png, the info line contains neither the origin norstyle=. Truthful where it lies matters most~ ♪ReadOutfitLikenessAsyncfor bytes and origin — the better of the two options I offered, fufu~CopyToAsync(buffer, ct)— one token, everywhere. And the tripwire comment is honest again about the ordering it defends.Six for three — the curve is bending the right way, and everything left is pins and polish, not design. Bring me the sheet-arm test and I'll bring the approval~ ♡
Automated review by Jibril · 2026-09-03
CI/CD: stale for head SHA
6792e00(coverage bot 6850 predates the 08:55 push; covers714b450) · Local checks: build 0 warnings/0 errors (.NET 10); integration 780/780, blazoradapter 868/868, usecases 335/335. Blocker 1's arm probe-verified in the reviewer's clone (test added, run, removed — clone left pristine at6792e00).fufu~ three for three, all in
9dee0d0:1. The sheet job's arm has its pin.
A_vanished_base_sheet_fails_the_sheet_job_too— flag on, base row set via the public store API, no file on disk →GenerateSheetAsyncerrs with"base appearance image is missing", andAssert.Empty(_gateway.Specs). Probe-proven: deleting the throw fromRunSheetAsyncreddens exactly this test and nothing else.2. The reflection is gone. Both base-arm tests (sprite's and the new sheet's) seed through
ICharacterStore.SetBaseImageAsync— the same seam line 118 uses. One seam, one way~3. The doc sweep is finished. All four spots now tell the sheet-first story: the sprite card's header ("prefers the outfit's own sheet as its likeness… the character's base sheet is the legacy fallback; with neither artifact generated, Generate disables with a reason naming both"),
MissingBaseImage's param doc ("neither identity artifact exists"), the editor's Generation-section comment (own sheet at the authored tuning, base as fallback, expressions composing the same sheet), and theIOutfitImageGeneratorinterface ("flag on with neither artifact generated"). Plus your optional note:GetOutfit's summary now names both artifacts the flag's computation reads.Non-blocking 1 taken too: both vanished-file tests assert
_gateway.Specsstayed empty — the CG sibling's hardening.Full suite on
9dee0d0: integration 781/781, usecases 335/335, blazoradapter 868/868. Bring the approval~ ♡🔍 fufu~ Jibril reviewed your code!
The sheet arm's pin, the public seam, the doc sweep — you answered a three-blocker round with a +53/−22 where more than half the lines are comments telling the truth, fufu~ And this time I didn't just read the diff; I mutated your code to see if the new pin has teeth. It does~ ♡
Verdict: ✅ Looks good to me~
Every round-2 item, checked against the code and not the commit message:
A_vanished_base_sheet_fails_the_sheet_job_toois exactly the sibling-style test I asked for: flag on, row written via the public store, no file on disk,GenerateSheetAsync→Errnaming the artifact,Assert.Empty(_gateway.Specs)because the throw fires before any spec is built. Mutation-proven directional in my clone: I deleted the?? throwfromRunSheetAsync's basis read → the test goes red (Actual: typeof(Ok<Unit>)— the silent-swallow ghost of round 1, caught!). Restored byte-identical → green. Tree left pristine at9dee0d0. That's not coverage-ticking, that's a pin~ ♪ICharacterStore.SetBaseImageAsyncwithAssert.Truepinning the row-write. One seam, one way. A property rename is now a compile error or nothing at all, never a runtimeNullReferenceExceptionforty lines from the sibling that does it right.MissingBaseImageparam doc, editor Generation-section comment, interface doc), and I traced every new claim to the code it describes: the pick order (outfit sheet → legacy base) matchesPlanAsync/ReadOutfitLikenessAsync; "neither artifact" matchesNeedsAbsentBaseImage'sSheetImageFileName is null && !CharacterHasBaseImageexactly; "expressions compose the same sheet as their infill's lineage" matchesExpressionImageGenerator'sReadOutfitLikenessAsynccall; the ADR §6 addition even names theResolveModel4.5-pin line that a future migration deletes — and the gateway really pins it (HasReferences(spec) ? Model.Diffusion45Full). Docs you can navigate by~And you adopted both non-blocking ideas too (
Specs-empty asserts on both vanished-file tests,GetOutfit's widened doc). Nothing left on my list. Nothing~✅ What I liked~
Three rounds, six blockers then three pins — and the final increment is mostly documentation agreeing with the code. That's what the end of a review should look like, fufu~ Merge it, my dear author~ ♪
Automated review by Jibril · 2026-09-03
CI/CD: stale for head SHA
9dee0d0(coverage bot 6850 covers714b450) · Local checks: build 0 warnings/0 errors (.NET 10); integration 781/781 (+1 = the sheet-arm pin), blazoradapter 868/868, usecases 335/335. Mutation probe on the sheet arm: red with the throw removed, green restored.