From f3ef7baae2b9ac67d6875dcdda4b32a1ccecf648 Mon Sep 17 00:00:00 2001 From: Erik Date: Sat, 15 Aug 2026 13:41:37 +0200 Subject: [PATCH] =?UTF-8?q?docs+fix(chargen):=20CC1/CC2=20review=20closeou?= =?UTF-8?q?t=20=E2=80=94=20R2/R3=20residuals=20closed,=20ledger=20final?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both narrow re-reviews returned CLOSED. This closeout takes the two cheap re-review residuals before CC3 takes references to the shared model: R2: every array handed into the typed chargen model is now wrapped in Array.AsReadOnly at the projection seam — a T[] behind IReadOnlyList was still downcast-mutable, and ChargenOptions is a process-shared singleton graph. R3: the no-Chorizite-leak guard now also walks public fields; every current type uses properties, but a public field would have slipped through the property-only walk. Ledger: CC1 fix-round sha corrected to cb4703e8 (the cell previously cited the pre-amend 459a87f2), CC1/CC2 rows flipped to REVIEW-CLOSED with the re-review outcomes, R1 (retail refunds +1 credit on a both-tier cost miss; port charges 0 — unreachable via retail's own listbox, noted for CC3) and the Olthoi-locked-to-template-0 decomp fact recorded for CC3/CC4. Core.Tests 4736/1 skip, Content.Tests 145/0, Release. Co-Authored-By: Claude Fable 5 --- .../2026-08-15-character-creation-campaign.md | 4 +- .../CharGen/ChargenTableReader.cs | 40 ++++++++++--------- .../CharGen/ChargenNoChoriziteLeakTests.cs | 17 ++++++-- 3 files changed, 37 insertions(+), 24 deletions(-) diff --git a/docs/plans/2026-08-15-character-creation-campaign.md b/docs/plans/2026-08-15-character-creation-campaign.md index 67d93676..b5ef1fdb 100644 --- a/docs/plans/2026-08-15-character-creation-campaign.md +++ b/docs/plans/2026-08-15-character-creation-campaign.md @@ -228,8 +228,8 @@ the user gate. | Slice | Status | Commits | Review | Notes | |---|---|---|---|---| -| CC1 | implemented; review fix round complete | `04450041`, `459a87f2` | dispatched 2026-08-15 | Core model (no Chorizite leak) + Content projector; 31 math units + 6 installed-DAT gates (13 heritages). FINDING for CC3: each human heritage's "Adventurer" template IS retail's Custom entry point — attributes at the 10-floor (60/330), a real TemplateCG row, not a UI special case. **Review fix round (`459a87f2`):** F1 doc corrected — Custom IS template index 0 (the Adventurer row), per `gmCGProfessionPage::UpdateProfession @ 0x004821b0` (case 0 → button 0x100003d9 / `ID_CharGen_CustomText`) and `CharGenState::SetTemplate @ 0x005C5A60` (commits via `CharGenState::ApplyTemplate @ 0x005C5080`, i.e. selecting Custom resets sliders to the floor spread, it does not bypass templates); F2 two-tier skill-cost fallback implemented (`ChargenOptions.GlobalSkillCostsBySkillId` from portal.dat 0x0E000004, `ChargenSkillCreditMath` checks heritage list then global list) + installed-DAT completeness assertion recording reality: the global SkillTable prices 38/54 advancement skill ids, every one of the 13 heritages ships EXACTLY one heritage-specific override (always also present in the global table), and 16 skill ids are genuinely uncostable in both tiers (retail's -1 case) — see `ChargenTableReaderInstalledDatTests.InstalledHeritages_SkillCostFallbackCoversTheKnownUncostableSkillSet`; F3 every `ChargenTableReader` collection is now frozen at projection (`ToFrozenDictionary`/`ToArray`, matching `MagicCatalog`'s pattern) including both `ChargenOptions.Empty` dictionaries; F4 a reflection guard test (`ChargenNoChoriziteLeakTests`) pins the no-Chorizite-leak contract by walking every public `AcDream.Core.CharGen` member; F5 `HasAnyAppearanceOptions`'s doc reworded to state precisely what it proves (an OR across eight lists, omitting the three color lists) + a new installed-DAT gate records per-list reality — found COMPLETE, every gender of every heritage has non-empty lists across all eight plus the three color lists, even the sparse Gear Knight/Olthoi variants; F6 `TryGetHeritage`/`TryGetStarterArea` annotated `[MaybeNullWhen(false)]` (matching the house `EmptyDatReaderWriter` pattern), all affected call sites (more than the originally estimated five) fixed across both test projects. Filed CC7 risk item 8: ACE's `PlayerFactory` heritage-override branch over-deducts skill credits when specializing a heritage-priced skill (references/ACE/Source/ACE.Server/Factories/PlayerFactory.cs:184-211) — a retail-legal build may be rejected by local ACE at the CC7 connected gate; this is an ACE bug, not an acdream defect. | -| CC2 | implemented (campaign-cc2); review in flight | `5eaad2c8` | dispatched 2026-08-15 | Byte-exact 0xF656 (19-term checksum vs CG_Pack accumulator), shared 0xF643 type, correlation latch, status events + contract amendment. Core.Net 993 / Runtime 1667 / Launcher.Core 323, Windows+WSL | +| CC1 | REVIEW-CLOSED 2026-08-15 | `04450041`, `cb4703e8` | CLOSED (fix round + narrow re-review; every citation independently re-derived) | Core model (no Chorizite leak) + Content projector; 31 math units + 6 installed-DAT gates (13 heritages). FINDING for CC3: each human heritage's "Adventurer" template IS retail's Custom entry point — attributes at the 10-floor (60/330), a real TemplateCG row, not a UI special case. **Review fix round (`cb4703e8`):** F1 doc corrected — Custom IS template index 0 (the Adventurer row), per `gmCGProfessionPage::UpdateProfession @ 0x004821b0` (case 0 → button 0x100003d9 / `ID_CharGen_CustomText`) and `CharGenState::SetTemplate @ 0x005C5A60` (commits via `CharGenState::ApplyTemplate @ 0x005C5080`, i.e. selecting Custom resets sliders to the floor spread, it does not bypass templates); F2 two-tier skill-cost fallback implemented (`ChargenOptions.GlobalSkillCostsBySkillId` from portal.dat 0x0E000004, `ChargenSkillCreditMath` checks heritage list then global list) + installed-DAT completeness assertion recording reality: the global SkillTable prices 38/54 advancement skill ids, every one of the 13 heritages ships EXACTLY one heritage-specific override (always also present in the global table), and 16 skill ids are genuinely uncostable in both tiers (retail's -1 case) — see `ChargenTableReaderInstalledDatTests.InstalledHeritages_SkillCostFallbackCoversTheKnownUncostableSkillSet`; F3 every `ChargenTableReader` collection is now frozen at projection (`ToFrozenDictionary`/`ToArray`, matching `MagicCatalog`'s pattern) including both `ChargenOptions.Empty` dictionaries; F4 a reflection guard test (`ChargenNoChoriziteLeakTests`) pins the no-Chorizite-leak contract by walking every public `AcDream.Core.CharGen` member; F5 `HasAnyAppearanceOptions`'s doc reworded to state precisely what it proves (an OR across eight lists, omitting the three color lists) + a new installed-DAT gate records per-list reality — found COMPLETE, every gender of every heritage has non-empty lists across all eight plus the three color lists, even the sparse Gear Knight/Olthoi variants; F6 `TryGetHeritage`/`TryGetStarterArea` annotated `[MaybeNullWhen(false)]` (matching the house `EmptyDatReaderWriter` pattern), all affected call sites (more than the originally estimated five) fixed across both test projects. Filed CC7 risk item 8: ACE's `PlayerFactory` heritage-override branch over-deducts skill credits when specializing a heritage-priced skill (references/ACE/Source/ACE.Server/Factories/PlayerFactory.cs:184-211) — a retail-legal build may be rejected by local ACE at the CC7 connected gate; this is an ACE bug, not an acdream defect. **Narrow re-review CLOSED:** the reviewer retro-graded F2 to HIGH (under the base commit 37 of 38 costable skills were charged zero) and confirmed the SkillBase.SpecializedCost->PrimaryCost mapping dodged the UpgradeCostFromTrainedToSpecialized trap. Residuals: R1 retail refunds +1 credit on a both-tier miss (port charges 0; unreachable via retail’s own skills listbox — NOTE FOR CC3 if any path ever exposes the 16 uncostable ids); R2 list downcast-mutability and R3 field-walking in the leak guard CLOSED at the merge-closeout commit (Array.AsReadOnly at every projection seam; GetFields walk added). Decomp fact for CC4: ApplyTemplate force-sets template_=0 for heritage 0xc/0xd — both Olthoi variants are hard-locked to Custom/template 0. | +| CC2 | REVIEW-CLOSED, MERGED 2026-08-15 (`55fc51ed`) | `5eaad2c8`, `e77ebf10`, `95e95bb6` | PASS then CLOSED (fix round: F1 latch-scope narrowing + overwrite pin test, F2 register AD-100, F3 ACE double-NameInUse note, F4 creationFailed{code,reason,name}, F5 pointer, retail-discriminator citations) | Byte-exact 0xF656 (19-term checksum vs CG_Pack accumulator), shared 0xF643 type, correlation latch, status events + contract amendment. Core.Net 993 / Runtime 1667 / Launcher.Core 323, Windows+WSL | | CC3 | — | | | | | CC4 | — | | | | | CC5 | — | | | | diff --git a/src/AcDream.Content/CharGen/ChargenTableReader.cs b/src/AcDream.Content/CharGen/ChargenTableReader.cs index 7426e539..127a215f 100644 --- a/src/AcDream.Content/CharGen/ChargenTableReader.cs +++ b/src/AcDream.Content/CharGen/ChargenTableReader.cs @@ -16,7 +16,7 @@ namespace AcDream.Content.CharGen; /// presentation-free tree. /// Mirrors MagicCatalog.Load's shape: one static entry point over /// , every returned collection is frozen at -/// projection (ToFrozenDictionary / ToArray, matching +/// projection (ToFrozenDictionary / Array.AsReadOnly, matching /// MagicCatalog's pattern), and no Chorizite types cross into the /// returned model. Cross-checked against ACE's /// ACE.DatLoader.FileTypes.CharGen + @@ -94,7 +94,7 @@ public static class ChargenTableReader } return new ChargenOptions( - starterAreas, + Array.AsReadOnly(starterAreas), heritagesById.ToFrozenDictionary(), globalSkillCosts.ToFrozenDictionary()); } @@ -110,7 +110,7 @@ public static class ChargenTableReader position.Frame.Origin, position.Frame.Orientation); } - return new ChargenStarterArea(index, area.Name.Value, locations); + return new ChargenStarterArea(index, area.Name.Value, Array.AsReadOnly(locations)); } private static ChargenHeritageOptions ProjectHeritage(uint heritageId, HeritageGroupCG cg) @@ -138,10 +138,10 @@ public static class ChargenTableReader cg.EnvironmentSetupId.DataId, cg.AttributeCredits, cg.SkillCredits, - cg.PrimaryStartAreas.ToArray(), - cg.SecondaryStartAreas.ToArray(), + Array.AsReadOnly(cg.PrimaryStartAreas.ToArray()), + Array.AsReadOnly(cg.SecondaryStartAreas.ToArray()), skillCosts.ToFrozenDictionary(), - templates, + Array.AsReadOnly(templates), gendersByKey.ToFrozenDictionary()); } @@ -166,8 +166,8 @@ public static class ChargenTableReader template.Quickness, template.Focus, template.Self), - normalSkills, - primarySkills); + Array.AsReadOnly(normalSkills), + Array.AsReadOnly(primarySkills)); } private static ChargenGenderOptions ProjectGender(int genderKey, SexCG sex) @@ -221,20 +221,20 @@ public static class ChargenTableReader sex.MotionTable.DataId, sex.CombatTable.DataId, ProjectObjDesc(sex.BaseObjDesc), - sex.HairColors.ToArray(), - hairStyles, - sex.EyeColors.ToArray(), - eyeStrips, - noseStrips, - mouthStrips, + Array.AsReadOnly(sex.HairColors.ToArray()), + Array.AsReadOnly(hairStyles), + Array.AsReadOnly(sex.EyeColors.ToArray()), + Array.AsReadOnly(eyeStrips), + Array.AsReadOnly(noseStrips), + Array.AsReadOnly(mouthStrips), ProjectGearList(sex.Headgears), ProjectGearList(sex.Shirts), ProjectGearList(sex.Pants), ProjectGearList(sex.Footwear), - sex.ClothingColors.ToArray()); + Array.AsReadOnly(sex.ClothingColors.ToArray())); } - private static ChargenGearOption[] ProjectGearList(List gearList) + private static IReadOnlyList ProjectGearList(List gearList) { var result = new ChargenGearOption[gearList.Count]; for (int i = 0; i < gearList.Count; i++) @@ -242,7 +242,7 @@ public static class ChargenTableReader GearCG gear = gearList[i]; result[i] = new ChargenGearOption(gear.Name.Value, gear.ClothingTable.DataId, gear.WeenieDefault); } - return result; + return Array.AsReadOnly(result); } private static CoreChargenObjDesc ProjectObjDesc(DatObjDesc objDesc) @@ -271,6 +271,10 @@ public static class ChargenTableReader animPartChanges[i] = new ChargenAnimPartChange(change.PartIndex, change.PartId.DataId); } - return new CoreChargenObjDesc(objDesc.PaletteId.DataId, subPalettes, textureChanges, animPartChanges); + return new CoreChargenObjDesc( + objDesc.PaletteId.DataId, + Array.AsReadOnly(subPalettes), + Array.AsReadOnly(textureChanges), + Array.AsReadOnly(animPartChanges)); } } diff --git a/tests/AcDream.Core.Tests/CharGen/ChargenNoChoriziteLeakTests.cs b/tests/AcDream.Core.Tests/CharGen/ChargenNoChoriziteLeakTests.cs index b055a96a..cfe7f4ac 100644 --- a/tests/AcDream.Core.Tests/CharGen/ChargenNoChoriziteLeakTests.cs +++ b/tests/AcDream.Core.Tests/CharGen/ChargenNoChoriziteLeakTests.cs @@ -11,10 +11,13 @@ namespace AcDream.Core.Tests.CharGen; /// future edit from putting e.g. a DatReaderWriter.Enums.SkillId /// directly on a public property. This test walks every public type in the /// AcDream.Core.CharGen namespace and asserts that no public -/// property, indexer, constructor parameter, or method return/parameter -/// type — nor any of their generic type arguments, recursively — comes -/// from the DatReaderWriter assembly or any assembly whose name -/// starts with Chorizite. +/// property, field, indexer, constructor parameter, or method +/// return/parameter type — nor any of their generic type arguments, +/// recursively — comes from the DatReaderWriter assembly or any +/// assembly whose name starts with Chorizite. (The field walk +/// closes the CC1 re-review's R3 residual: every current type uses +/// properties, but public SkillId Foo; would otherwise slip +/// through.) /// public sealed class ChargenNoChoriziteLeakTests { @@ -51,6 +54,12 @@ public sealed class ChargenNoChoriziteLeakTests } } + foreach (FieldInfo field in type.GetFields( + BindingFlags.Public | BindingFlags.Instance | BindingFlags.Static)) + { + CheckSite(field.FieldType, $"{type.FullName}.{field.Name} (field)", offenders); + } + foreach (ConstructorInfo ctor in type.GetConstructors( BindingFlags.Public | BindingFlags.Instance)) {