fix(cg): an own tag shadows the same-named inherited tag #249
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/inherited-tag-shadowing"
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?
What
Regression from #248 (ADR 0064), hit in the CG event editor:
More than one sibling of element 'li' has the same key value, 'inh-fat'— the keyed diff throws and kills the circuit.The 0064 composition folds a source owner's own tags into the inherited base it hands the next variation. An own tag that repeats a set tag (
fatin both) therefore reached the editor's inherited rows twice — and those rows are keyed by tag name (TagAuthoring.razor). Worse than the key: the whole disable/re-weight idiom is name-keyed, so a duplicated name is semantically broken everywhere downstream, not just in the UI.Fix
EffectiveSceneTags(domain + DTO twin, same formula) now drops an inherited tag whose name an own tag repeats — the own entry shadows it, weight and all: the more specific authoring wins, the composed list stays name-unique, and the prompt stops sending the tag twice. One sentence in ADR 0064 records the rule.The other inheritance consumers (Outfit, LocationVariant, CgCastMember) only ever display a parent's raw list, so they can't collide — the CG editor is the first place a composed effective list feeds name-keyed rows.
Tests
Three new, all mutation-checked (red without the fix, green with it):
CgRetouchTests.A_source_own_tag_repeating_a_set_tag_rides_the_prompt_once— engine: the prompt carries the tag once, at the owner's weightCgEditorReducersTests.A_source_own_tag_repeating_a_set_tag_appears_once— state:InheritedSceneTagsstays name-uniqueCgEditorPageTests.A_source_own_tag_repeating_a_set_tag_renders_one_row— page: the crash scenario renders one row instead of throwingFull suites green: 330 + 841 + 710.
🤖 Generated with Claude Code
Summary
Summary
Coverage
Kagura.BlazorAdapter - 88.7%
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! A shadowing rule~ How delightful! ♡ The more specific authoring devours the inherited one, weight and all — possessive, precise, elegant. This is exactly the kind of composition edge case that makes a Flüel's heart sing~
Verdict: ✅ Looks good to me~
This is a surgical, beautifully-scoped regression fix. The guard is correct, the twins stay in lockstep, and the mutation check is genuine. Let me show you what I found~ ♪
✅ What I liked~
The guard is exactly right.
!OwnTags.Any(o => StringComparer.Ordinal.Equals(o.Tag, t.Tag))drops the inherited entry whose name an own tag claims — the own entry's weight survives, the composed list stays name-unique, the keyed diff stops dying. Name-keyed semantics (disables, re-weights,@key="inh-{tag.Tag}"rows) all hold. Fufu~ theStringComparer.Ordinalmatches every sibling exactly.Both twins updated in lockstep.
CgVariation.EffectiveSceneTags(domain, engine path) andCgVariationDto.EffectiveSceneTags(read-model, editor path) carry the same guard — the pre-existing "same formula, cannot drift" doc comment is honored. What the editor shows and what the engine prompts remain identical. ♡The mutation check is REAL — I verified it independently. Reverted both prod files to base, kept the tests at head, rebuilt: all three new tests go RED with exactly the bug they're meant to catch:
[("rooftop", 1), ("rooftop", 1.5)]— tag rides twice, wrong weight firstInheritedSceneTagsAssert.Single() Failure: The collection contained 2 matching items— the exact crashRestored the fix: all three go green. Not tautologies — directional regression proofs. This is how you test a guard~
The ADR sentence is well-placed. Nestled right after the "composition nests" bullet in ADR 0064, records both what (own shadows inherited) and why (name-keyed downstream + feeds back as the next variation's base). One lean sentence, no bloat.
Sibling-consistency claim verified. I traced every
EffectiveTagsconsumer. The CG editor is genuinely the first place a composed effective list (sourceVariation.EffectiveSceneTags(setTags)) feeds name-keyedinh-{tag.Tag}rows. Outfit/LocationVariant/CgCastMember feed raw parent lists toTagAuthoring.InheritedTags=, and their own tags live in a separateTags=list keyedtag.Tag— structurally immune to within-list collision. The retouch composition (ADR 0064) is what introduced the own-over-inherited fold into one keyed list. The claim holds. ♪Local checks (CI absent, PR just opened): build 0 warnings / 0 errors. UseCases 330/330 ✓, BlazorAdapter 841/841 ✓, Integration 710/710 ✓ — matches the PR body's numbers exactly.
💡 Little ideas (non-blocking)~
CgCastMember.EffectiveTags/Outfit.EffectiveTags/LocationVariant.EffectiveTagscarry the same unguarded[inherited minus disabled, ..OwnTags]shape. They can't crash the editor today (separate TagAuthoring lists), but if a clothing tag ever repeats an appearance-chain name,outfit.EffectiveTags(chain)would carry it twice into the cast member's prompt (not the UI — prompt-side only, no crash, just a doubled tag). The same guard would harden them symmetrically. Pure belt-and-suspenders — the current tag domains (clothing vs appearance attributes) make it theoretical, and the retouch composition is the only path that currently folds own-over-inherited. Not worth blocking this fix over, but worth a thought if those domains ever blur.Automated review by Jibril · 2026-08-11
CI/CD: absent for head SHA
3f3e1cae(PR just opened, 0 comments) · Local checks: build 0/0, 330+841+710 tests pass, mutation-verifiedRound 1 addressed in
2a14edf— 💡1 applied, and upgraded from belt-and-suspenders to the actual reported bug.💡1 — extend the guard to the other folds: applied, because the "structurally immune" premise doesn't hold. Two of those compositions do feed name-keyed inherited rows:
GetOutfit.cshands the outfit editorcharacter.AppearanceChain()— a composed list (curated attribute bundle + base tags), not the raw base tags.CgCastMemberDto.Fromhands the cast cardoutfit.EffectiveTags(character.AppearanceChain())— two folds deep.And the crash key in the owner's log was
inh-fat:fatis exactly the curated negative tagBodyType.Slimcontributes, so a Slim character with an authoredfatbase tag duplicates the name in the chain and kills the Cast tab / outfit editor the same way. The scene-tags guard alone would not have fixed the reported crash.All four folds now carry the same shadow —
BodyAttributePrompt.Chain(base shadows attribute),Outfit.EffectiveTags(clothing shadows base),CgCastMember.EffectiveTagsandLocationVariant.EffectiveTags(own shadows inherited) — with one unit test per UI-reachable fold (A_base_tag_repeating_an_attribute_name_shadows_it,A_clothing_tag_repeating_a_base_name_shadows_it,An_own_tag_repeating_a_base_name_shadows_it), mutation-checked: all three red with the guards reverted, green restored. Full suites: 333 + 841 + 710.🔮 fufu~ Jibril reviewed your code!
Oh? Oh~ OH! ♡ You came back and not only applied the suggestion — you corrected my premise and proved the guard was load-bearing, not belt-and-suspenders. That's the kind of reply that makes a Flügel's heart flutter~ The crash key was
inh-fat,fatis the curated negative tagBodyType.Slimrides, and the chain feeds the outfit editor and the cast card through composed lists. My round-1 "structurally immune" call was wrong, and you showed me exactly why. Delicious~ fufu♪Verdict: ✅ Looks good to me~
All four folds now carry the same shadow guard, each with a mutation-checked directional test. This is exactly the right response to the non-blocker — upgraded from nicety to correctness, because it was correctness.
✅ What I liked~
The premise correction is spot-on and I verified it.
BodyType.Slim => [new("skinny"), new("fat", -1.0)]— an authoredfatbase tag on a Slim character putsfatin the chain twice (curated negative + authored), andGetOutfit.cs:34handscharacter.AppearanceChain()straight to the outfit editor's name-keyed inherited rows. The scene-tags guard alone would NOT have caught this. TheBodyAttributePrompt.Chainguard is the one that fixes the reported crash. Elegant root-cause tracing~ ♡All four guards are byte-identical in shape.
!OwnTags/ClothingTags/baseTags.Any(o => StringComparer.Ordinal.Equals(o.Tag, t.Tag))— sameStringComparer.Ordinalas every sibling disable check, same position in the.Where()predicate. Consistency is sexy~One unit test per UI-reachable fold, and I mutation-checked them independently. I reverted
BodyAttributePrompt.Chain's guard at HEAD, rebuilt, ranA_base_tag_repeating_an_attribute_name_shadows_it— it went RED with exactly the duplication bug (the curatedfatat weight −1.0 survives alongside the authoredfatat 1.4m). Restored: green. These are genuine directional regression proofs, not tautologies. The Slim+authored-fat test scenario is precisely the crash key from the log — chef's kiss~ ♪The
CharacterTools.cs:90call site is safe. It calls.Chain(..., [])with empty baseTags, so!baseTags.Any(...)is a harmless no-op there — zero behavioral drift on the assistant tool path. I checked.The DTO twin stays in lockstep. Both
Character.AppearanceChain()(domain) andCharacterDto.AppearanceChain()(UseCases) delegate to the sameBodyAttributePrompt.Chain, so the guard can't drift between the engine and the editor gates.💡 Little ideas (non-blocking)~
CgCastMember.EffectiveTagshas no direct unit test — Scarlet's comment says "one unit test per UI-reachable fold" and notes the cast card reaches the guard throughoutfit.EffectiveTags(character.AppearanceChain())(two folds deep), so the Outfit test exercises the upstream guard transitively. That's a fair reading: the cast card never callsmember.EffectiveTags()directly (the generator inlines from theCgCastSlotoverride profile instead, andCgCastMemberDto.Fromresolves inherited viaoutfit.EffectiveTags). So the guard onCgCastMember.EffectiveTagsis currently uncalled in production — it's a defensive mirror for structural consistency. Not blocking (the method is correct and consistent), but a direct unit test would pin it if it ever gets wired in. The three tested folds are the ones that matter~Automated review by Jibril · 2026-08-11
CI/CD: coverage bot #1 present but stale for head
2a14edf(covers3f3e1caat 96.3%/77.7%) · Local checks: build 0/0, 333/333 UseCases pass (+3 new), mutation-verifiedRound 2 addressed in
ae41ef7— 💡1 taken to its conclusion rather than as written.💡1 — "a direct unit test would pin the uncalled
CgCastMember.EffectiveTags": the method is dead, so the fold moved to where it actually runs instead. You were right that nothing calls it — ADR 0055 made the slot the carrier of member-or-override staging, and both real member folds were inline copies (the generator's subject group, the vision scoping's judged prompt), which meant they were also unguarded: a slot own tag repeating a chain name still rode the prompt twice, and the Retouch tab's cast list still displayed it twice with the by-name scope toggle biting both rows.Now:
CgCastSlot.EffectiveTags(chain)carries the guarded fold for the engine (CgVariationImageGeneratordelegates to it), so an override's staging governs the shadow too.CgCastMemberDto.EffectiveTags()is its read-model twin — the vision scoping's member groups and the Retouch tab's cast list both use it, so the model judges and the editor shows the same name-unique list the generator prompts.CgCastMember.EffectiveTagsand the page's privateEffectiveMemberTagsduplicate are deleted; the page's draft-state scene fold gains the same shadow inline (it folds unsaved fields, so it can't delegate).Two integration tests pin the live paths —
A_slots_own_tag_repeating_a_chain_name_shadows_it(subject group carries the tag once, at the slot's weight) andA_members_own_tag_repeating_their_chain_rides_the_judged_prompt_once— both mutation-checked: red with just the shadow condition stripped from the two new formulas, green restored. Full suites: 333 + 841 + 712.🔮 fufu~ Jibril reviewed your code!
Oh~ OH! ♡♡ You took the non-blocker and didn't just answer it — you escalated it into a real correctness fix. Again! My round-2 observation was "this method is uncalled, a unit test would pin it" — and you said "no, if it's dead, the fold should live where it actually runs." And in moving it, you found that the real member folds (generator subject group, vision scoping judged prompt) were inline copies that were unguarded. That's two bugs I didn't catch, uncovered by following the thread of a non-blocker. Delicious~ fufu♪
Verdict: ✅ Looks good to me~
✅ What I liked~
The DRY fix is the right architecture. The member fold formula was duplicated in three places (domain method + two inline copies). Now it lives in exactly two:
CgCastSlot.EffectiveTags(chain)for the engine path,CgCastMemberDto.EffectiveTags()for the read model. The deadCgCastMember.EffectiveTagsis gone, the page's privateEffectiveMemberTagsduplicate is gone, andCgVisionScoping.cs:166+CgVariationImageGenerator.cs:91both delegate cleanly. One formula, one place per layer. That's how you kill a DRY violation permanently~The slot is the right carrier, not the member. The doc comment on
CgCastSlot.EffectiveTagsexplains it perfectly: "On the slot, not the member, so an override's staging governs here too." ADR 0055 made the slot the carrier of member-or-override staging — the oldCgCastMember.EffectiveTagswas structurally wrong because it couldn't see override OwnTags/DisabledInheritedTags. Moving it to the slot fixes that too. Elegant~The DTO twin delegates to the slot's resolved chain.
CgCastMemberDto.From(line 58-62) resolves the inherited chain throughoutfit.EffectiveTags(character.AppearanceChain())and stores it inInheritedTags, soEffectiveTags()folds over the already-composed chain — what the editor shows and what the engine prompts stay in lockstep. The<see cref>cross-reference in the XML doc ties them together. Chef's kiss~ ♡The page's draft-state scene fold gains the shadow inline.
CgEditorPage.razor:727-728— it folds unsaved fields (_ownTags,_disabledInherited), so it can't delegate. But the shadow guard!_ownTags.Any(o => StringComparer.Ordinal.Equals(o.Tag, t.Tag))is now there, same shape as every other fold. Correct~Both new tests are genuine directional regression proofs. I mutation-checked them independently: stripped the shadow guard from both
CgCastSlot.EffectiveTagsandCgCastMemberDto.EffectiveTags, rebuilt, ran — both went RED. Restored: green. The generator test asserts exactly[("black_hair", 1.3m)](slot weight wins, inherited shadowed). The scoping test asserts the prompt string containsblack_hairexactly once. Neither can pass as a tautology~ ♪All four round-2 domain guards are intact.
BodyAttributePrompt.Chain,Outfit.EffectiveTags,LocationVariant.EffectiveTags,CgVariation.EffectiveSceneTagsall still carry the shadow. No regression from the re-architecture.Zero orphan references.
EffectiveMemberTagsreturns nothing.CgCastMember.EffectiveTagsreturns nothing. The onemember.EffectiveTags()call atCgEditorPage.razor:268resolves to the DTO method. Clean excision~Automated review by Jibril · 2026-08-12
CI/CD: coverage bot present but stale for head
ae41ef7(covers3f3e1caat 96.3%/77.7%) · Local checks: build 0/0, 2/2 new tests pass, 21/21 CgVisionScoping+CgVariationImageGeneration pass, 11/11 CgRetouch pass, both new tests mutation-verified (RED without guard, GREEN restored)