fix(chargen): Campaign CC CC5 re-review residuals R1-R5 + nits — REVIEW-CLOSED

The narrow re-review of fix commits 0c8e1e7d+2d4168f9 found every code fix
oracle-verified but returned NOT CLOSED on five test/doc residuals plus
three nits and a follow-up filing. All fixed:

- R1: the claimed "external-change->user-commit->SetName" regression test
  for F1 (the deleted _suppressNextFieldEvent latch) never existed — the
  Runtime-layer randomize test doesn't touch the page. Added
  CharacterCreationUiControllerTests.SummaryNameField_RealCommitAfter
  ExternalRefreshWhileUnfocused_StillReachesSetName: drives Refresh with a
  revision bump + changed snapshot.Name while the field is unfocused (the
  programmatic SetText path that used to arm the latch), THEN performs a
  real user commit (field.SetText + field.Submit(), the actual event path),
  asserting SetName receives the player's typed text.
- R2: RetailSkillFormula.CalculateChargenScore and ChargenSkillScoreResolver
  had zero direct coverage (the F12(d) test substitutes skillId*10). Added
  a Untrained/Trained(+5)/Specialized(+10) theory, the divisor-zero skip
  path, and a six-way AttributeId theory (Str=1..Self=6) to
  RetailSkillFormulaTests.cs.
- R3: RetailSkillFormula.cs's doc comment claimed "no retail-authored skill
  sets MinLevel above Untrained=1" without ever reading the field — ACE's
  own SkillBase.cs hedges the same field "// 1-2?". MEASURED (not assumed)
  against the installed EoR dat's global SkillTable
  (CharacterCreationLiveDatTests.SkillTable_MinLevelDistribution_
  NeverExceedsTrained): 23 skills at MinLevel 1, 15 at MinLevel 2, zero
  above 2, of 38 priced skills. ACE's hedge was right; the doc comment now
  states the measured fact and leans on the structural argument (the gate
  holds for Trained/Specialized under any MinLevel in {1,2}) as load-
  bearing, not the unverified data claim.
- R4: filed AP-228 — the Summary/Skills skill-row KEY sources from
  ItemAppraisalTextFormatter.SkillName's hardcoded English switch, where
  retail's own key is DAT-sourced (SkillBase->_name via %hs,
  0x0047b90f-0x0047b915) — same divergence class as AP-226 filed the same
  round, reversed polarity, also present at CC4's Skills page. Softened
  AP-224's "ported exactly, not simplified" claim: it only ever covered the
  row's VALUE/template, never its KEY.
- R5: this commit corrects 0c8e1e7d's gate claim. "Release build zero
  warnings" was false: a clean `dotnet build -c Release -t:Rebuild` shows
  25 pre-existing warnings (18 in tests/AcDream.Core.Tests, 7 in
  tests/AcDream.App.Tests — Composition/HostInputCameraCompositionTests.cs,
  Composition/WorldRenderCompositionTests.cs,
  UI/Layout/OptionsPanelLiveMountProbeTests.cs), none in any file this
  campaign or its residual round touched. History is not amended; this is
  the correction.

Nits: the ChargenPreviewController ctor doc now also cites
gmCGSummaryPage::Update @0x0047baa0 (the per-heritage re-derive site — 0xc
Olthoi/0xd OlthoiAcid/else — not just the one-shot InitializePage seed) as
the stronger justification for why Rebuild re-derives the zoomed-out eye
per heritage on every change. RuntimeCharacterCreationState's F2 comment
("Finish becoming a permanent no-op") reworded: the same unconditional
_verificationPending = false assignment ran pre-fix too, so Finish was
never blocked — only the response FEEDBACK vanished (no dialog, no created
character, nothing), not the request itself. Filed #404 for
ChargenSkillScoreResolver's own independent SkillTable read alongside
ChargenTableReader's (cleanup follow-up, out of this round's scope).

Ledger: CC5 flipped REVIEW-CLOSED in the campaign plan (dual-lens
architectural PASS-with-items / retail-fidelity FAIL -> F1-F14 fix round
0c8e1e7d -> narrow re-review: all code oracle-verified, residuals R1-R5
test/doc -> this commit; re-reviewer pre-authorized lead diff-check close).
This commit's own sha is recorded by a follow-up ledger-only commit,
matching 2d4168f9's own pattern.

Gates: Release build 0 errors (25 pre-existing warnings, unrelated to this
round — see R5 above); App suite 5257/3 skips (was 5242/3), 0 failed;
Runtime suite 1726/0 (unchanged); the three new/measured tests (R1, R2's
ten cases, R3) all pass individually.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

docs/ISSUES.md

@@ -24,6 +24,45 @@ What does NOT go here:
 - Every session: scan OPEN issues at start; promote/close anything we touched during the session before ending.
 - Promoting to a Phase: mark as `DONE (promoted to Phase X)` + commit SHA where the Phase entry landed.
 
+## #404 — ChargenSkillScoreResolver duplicates ChargenTableReader's own SkillTable read
+
+**Status:** OPEN (post-CC cleanup follow-up)
+**Severity:** LOW
+**Filed:** 2026-08-16 (Campaign CC CC5 re-review residual round, nit 3)
+**Component:** `src/AcDream.App/Composition/InteractionRetainedUiComposition.cs`
+(`ChargenSkillScoreResolver` construction, `:670-672`),
+`src/AcDream.Content/CharGen/ChargenTableReader.cs` (`:41`, `:61`)
+
+`ChargenSkillScoreResolver`'s constructor takes its OWN independent read of
+the global SkillTable (portal.dat `0x0E000004`) at composition time
+(`InteractionRetainedUiComposition.cs:670-672`,
+`d.Dats.Get<SkillTable>(0x0E000004u)`), beside `ChargenTableReader`'s
+own already-established read of the SAME table
+(`ChargenTableReader.cs:41` names the id, `:61` reads it) — which discards
+the DAT's `SkillFormula` field entirely (`ChargenTableReader.Project` only
+projects `TrainedCost`/`SpecializedCost` per skill into
+`ChargenSkillCost`, never `SkillBase.Formula`). Two independent reads of
+the same DAT file are harmless today (both are read-only, one-shot, under
+the DAT lock) but are a duplicate-source-of-truth smell: if the two readers
+ever diverge (a caching change, a future write path), nothing enforces they
+stay in sync.
+
+**Fix direction:** project `SkillFormula` (and `MinLevel`, needed by
+`RetailSkillFormula.CalculateChargenScore`'s gate) into `ChargenOptions`
+alongside the existing `GlobalSkillCostsBySkillId` — `ChargenTableReader`
+already walks every `SkillBase` in the table
+(`ChargenTableReader.Project`'s `globalSkillCosts` loop) so adding the
+formula/MinLevel costs no new DAT read, just a wider projection type. Then
+`ChargenSkillScoreResolver` becomes pure arithmetic over `ChargenOptions`
+it already receives from the caller, with no `SkillTable`/DAT dependency of
+its own, and its constructor-time DAT read goes away entirely.
+
+**Acceptance:** one SkillTable read at composition time (through
+`ChargenTableReader`), not two; `ChargenSkillScoreResolver` (or its
+replacement) takes `ChargenOptions`/a projected formula table instead of a
+raw `SkillTable`; existing `RetailSkillFormulaTests`/`ChargenTableReaderInstalledDatTests`
+coverage still passes.
+
 ## #403 — Consolidate RetailAnimationCyclePlayback into LiveEntityAnimationPresenter's legacy branch
 
 **Status:** OPEN (post-CC consolidation follow-up)

src/AcDream.App/Net/RetailSkillFormula.cs

@@ -48,11 +48,24 @@ internal static class RetailSkillFormula
     /// typed <c>SkillBase.MinLevel</c> field is the same value cleanly), is
     /// satisfied for both callers of this method (Specialized=3 and
     /// Trained=2 are the only two advancement classes CC5's Summary listbox
-    /// still shows — AP-224 — and no retail-authored skill sets
+    /// still shows — AP-224) FOR ANY <c>MinLevel</c> in <c>{1, 2}</c> — that
-    /// <c>MinLevel</c> above Untrained=1) so it is not reproduced as a
+    /// structural argument, not an assumption about the data, is what makes
-    /// separate branch; a future caller passing <see cref="ChargenSkillAdvancementClass.Untrained"/>
+    /// omitting the branch safe. CC5 re-review residual round, R3
-    /// or <see cref="ChargenSkillAdvancementClass.Inactive"/> would need
+    /// (2026-08-16), corrects the data claim this comment used to make in
-    /// that gate ported for real.
+    /// place of that argument ("no retail-authored skill sets MinLevel
+    /// above Untrained=1"): ACE's own <c>SkillBase.cs</c> annotates the
+    /// identical field <c>// 1-2?</c>, a hedge this port never checked.
+    /// MEASURED against the installed EoR dat's global SkillTable
+    /// (<see cref="AcDream.App.Tests.UI.Layout.CharacterCreationLiveDatTests.SkillTable_MinLevelDistribution_NeverExceedsTrained"/>):
+    /// of the 38 priced skills, 23 carry <c>MinLevel == 1</c> and 15 carry
+    /// <c>MinLevel == 2</c> — ACE's hedge was correct (real 2s exist), and
+    /// no skill in the installed table exceeds 2, so the gate stays
+    /// satisfied — but the ORIGINAL claim ("no skill sets MinLevel above
+    /// Untrained=1") was false as written, unrelated to whether omitting
+    /// the branch happens to still be safe. A future caller passing
+    /// <see cref="ChargenSkillAdvancementClass.Untrained"/> or
+    /// <see cref="ChargenSkillAdvancementClass.Inactive"/> would need that
+    /// gate ported for real regardless of MinLevel's observed range.
     /// </summary>
     public static uint CalculateChargenScore(
         SkillBase skillBase,

src/AcDream.App/Rendering/ChargenPreviewController.cs

@@ -204,9 +204,20 @@ internal sealed class ChargenPreviewController :
     /// eye literal <c>(0, -2.5, 0.95)</c> at <c>~0x0047bd14-0x0047bd44</c> —
     /// exactly <see cref="ChargenPreviewCamera.ResolveZoomedOutEye"/>'s
     /// default-heritage value, NOT the zoomed-in one this controller used
-    /// before the fix). Summary has no zoom buttons at all (retail's own
+    /// before the fix). CC5 re-review residual round, nit 1 (2026-08-16):
-    /// viewport there is fixed-framing), so this is a permanent camera
+    /// <c>InitializePage</c> alone only justifies the ONE-TIME seed below —
-    /// profile for the controller's whole lifetime, not a toggle.</param>
+    /// the STRONGER citation for why <see cref="Rebuild"/> re-derives this
+    /// same eye PER HERITAGE on every heritage/gender change (not just
+    /// once) is <c>gmCGSummaryPage::Update @ 0x0047baa0</c>, which re-sets
+    /// the camera on every update using the identical per-heritage mapping
+    /// <see cref="ChargenPreviewCamera.ResolveZoomedOutEye"/> already
+    /// implements (<c>0xc</c> Olthoi → <c>(0, -3.8, 1.15)</c>, <c>0xd</c>
+    /// OlthoiAcid → <c>(0, -5.7, 1.65)</c>, else → <c>(0, -2.5, 0.95)</c>) —
+    /// confirming the per-heritage re-derive below is retail-correct, not
+    /// an acdream-only elaboration on a one-shot init value. Summary has no
+    /// zoom buttons at all (retail's own viewport there is fixed-framing),
+    /// so this is a permanent camera profile for the controller's whole
+    /// lifetime, not a toggle.</param>
     public ChargenPreviewController(
         IChargenPreviewRenderer renderer,
         ChargenPreviewCamera camera,

src/AcDream.Runtime/Session/RuntimeCharacterCreationState.cs

@@ -1870,8 +1870,17 @@ public sealed class RuntimeCharacterCreationState : IDisposable
                 // Concrete effect: ACE's disabled-Olthoi Pending rejection
                 // (CharacterHandler.CharacterCreateEx's olthoi_play_disabled
                 // branch) now surfaces a visible dialog instead of silently
-                // resetting verification state with Finish becoming a
+                // resetting verification state. CC5 re-review residual
-                // permanent no-op.
+                // round, nit 2 (2026-08-16): "Finish becoming a permanent
+                // no-op" overstated the old symptom — this same
+                // unconditional `_verificationPending = false` assignment
+                // above ALSO ran pre-fix, so Finish was never blocked and
+                // each click still sent a fresh CharacterCreate request.
+                // What actually vanished was the FEEDBACK: every response
+                // produced no player-visible outcome at all (no dialog, no
+                // created character, nothing), so repeated Finish clicks
+                // looked identical to doing nothing, even though each one
+                // was a real round trip to ACE.
                 string reason = response.AsCode.ToString();
                 _lastRejection = new RuntimeCharacterCreationRejection(
                     response.RawCode,

tests/AcDream.App.Tests/Net/RetailSkillFormulaTests.cs

@@ -1,4 +1,5 @@
 using AcDream.App.Net;
+using AcDream.Core.CharGen;
 using DatReaderWriter.DBObjs;
 using DatReaderWriter.Enums;
 using DatReaderWriter.Types;
@@ -109,4 +110,104 @@ public sealed class RetailSkillFormulaTests
         Attribute2Multiplier = y,
         Divisor = unchecked((int)z),
     };
+
+    // ── CC5 re-review residual round, R2 (2026-08-16): RetailSkillFormula.
+    // ── CalculateChargenScore / ChargenSkillScoreResolver had zero direct
+    // ── coverage — the F12(d) test substitutes `skillId * 10` instead of
+    // ── exercising the real formula. ─────────────────────────────────────
+
+    /// <summary>
+    /// <see cref="RetailSkillFormula.CalculateChargenScore"/>'s level bonus:
+    /// Untrained adds nothing, Trained adds 5, Specialized adds 10, on top
+    /// of the SAME base <see cref="RetailSkillFormula.TryCalculate"/> result
+    /// (formula here: <c>w=0, x=1, y=0, z=1</c> against
+    /// <c>attribute1=5</c> -&gt; base 5).
+    /// </summary>
+    [Theory]
+    [InlineData(ChargenSkillAdvancementClass.Untrained, 5u)]
+    [InlineData(ChargenSkillAdvancementClass.Trained, 10u)]
+    [InlineData(ChargenSkillAdvancementClass.Specialized, 15u)]
+    public void CalculateChargenScore_AddsTheLevelBonusOnTopOfTheBaseFormula(
+        ChargenSkillAdvancementClass level,
+        uint expected)
+    {
+        var skillBase = new SkillBase { Formula = Formula(w: 0, x: 1, y: 0, z: 1) };
+
+        uint result = RetailSkillFormula.CalculateChargenScore(skillBase, attribute1: 5u, attribute2: 0u, level);
+
+        Assert.Equal(expected, result);
+    }
+
+    /// <summary>
+    /// The divisor-zero skip path: <see cref="RetailSkillFormula.TryCalculate"/>'s
+    /// own failure gate short-circuits <see cref="RetailSkillFormula.CalculateChargenScore"/>
+    /// to a flat 0 BEFORE the level bonus is ever added — even for
+    /// Specialized, which would otherwise add 10.
+    /// </summary>
+    [Fact]
+    public void CalculateChargenScore_ZeroDivisor_ReturnsZero_RegardlessOfLevel()
+    {
+        var skillBase = new SkillBase { Formula = Formula(w: 7, x: 1, y: 1, z: 0) };
+
+        Assert.Equal(0u, RetailSkillFormula.CalculateChargenScore(
+            skillBase, 10u, 20u, ChargenSkillAdvancementClass.Untrained));
+        Assert.Equal(0u, RetailSkillFormula.CalculateChargenScore(
+            skillBase, 10u, 20u, ChargenSkillAdvancementClass.Trained));
+        Assert.Equal(0u, RetailSkillFormula.CalculateChargenScore(
+            skillBase, 10u, 20u, ChargenSkillAdvancementClass.Specialized));
+    }
+
+    /// <summary>
+    /// <see cref="ChargenSkillScoreResolver.Resolve"/>'s six-way
+    /// <c>ResolveAttribute</c> switch — one case per
+    /// <see cref="AttributeId"/> (Strength=1..Self=6) — resolving through a
+    /// formula that reads ONLY <c>Attribute1</c> (<c>x=1, y=0</c>) so the
+    /// result unambiguously reports which of the six
+    /// <see cref="ChargenAttributeValues"/> fields the switch actually read.
+    /// </summary>
+    [Theory]
+    [InlineData(AttributeId.Strength)]
+    [InlineData(AttributeId.Endurance)]
+    [InlineData(AttributeId.Coordination)]
+    [InlineData(AttributeId.Quickness)]
+    [InlineData(AttributeId.Focus)]
+    [InlineData(AttributeId.Self)]
+    public void ChargenSkillScoreResolver_ResolvesEachAttributeIdThroughTheSixWaySwitch(
+        AttributeId attributeId)
+    {
+        const uint skillId = 0x10u;
+        var skillTable = new SkillTable();
+        skillTable.Skills.Add((SkillId)skillId, new SkillBase
+        {
+            Formula = new SkillFormula
+            {
+                AdditiveBonus = 0,
+                Attribute1Multiplier = 1,
+                Attribute2Multiplier = 0,
+                Divisor = 1,
+                Attribute1 = attributeId,
+                // Attribute2 deliberately left at its zero default (Strength) —
+                // Attribute2Multiplier=0 means whatever it reads contributes
+                // nothing, so it cannot mask a wrong Attribute1 case.
+            },
+        });
+        var resolver = new ChargenSkillScoreResolver(skillTable);
+        ChargenAttributeValues attributes = AttributeValuesWith(attributeId, 42);
+
+        uint result = resolver.Resolve(skillId, attributes, ChargenSkillAdvancementClass.Untrained);
+
+        Assert.Equal(42u, result);
+    }
+
+    private static ChargenAttributeValues AttributeValuesWith(AttributeId attributeId, int value) =>
+        attributeId switch
+        {
+            AttributeId.Strength => new ChargenAttributeValues(value, 0, 0, 0, 0, 0),
+            AttributeId.Endurance => new ChargenAttributeValues(0, value, 0, 0, 0, 0),
+            AttributeId.Coordination => new ChargenAttributeValues(0, 0, value, 0, 0, 0),
+            AttributeId.Quickness => new ChargenAttributeValues(0, 0, 0, value, 0, 0),
+            AttributeId.Focus => new ChargenAttributeValues(0, 0, 0, 0, value, 0),
+            AttributeId.Self => new ChargenAttributeValues(0, 0, 0, 0, 0, value),
+            _ => default,
+        };
 }

tests/AcDream.App.Tests/UI/Layout/CharacterCreationLiveDatTests.cs

@@ -572,6 +572,51 @@ public sealed class CharacterCreationLiveDatTests
         Assert.IsType<UiText>(UiElement.FindDescendant(pairRow, 0x100002FDu));
     }
 
+    /// <summary>
+    /// CC5 re-review residual round, R3 (2026-08-16): MEASURES the
+    /// installed global SkillTable's (portal.dat <c>0x0E000004</c>)
+    /// <c>MinLevel</c> distribution instead of inferring it — the C4
+    /// closeout's "observe, don't infer" lesson
+    /// (<c>docs/research/2026-08-05-c4-closeout-handoff.md</c>).
+    /// <see cref="AcDream.App.Net.RetailSkillFormula.CalculateChargenScore"/>'s
+    /// own doc comment used to claim "no retail-authored skill sets
+    /// MinLevel above Untrained=1" without ever reading the field; ACE's own
+    /// <c>SkillBase.cs</c> annotates the same field <c>// 1-2?</c> (a hedge
+    /// that observed 2s exist). This is that measurement — see the method's
+    /// own real-DAT finding recorded in <c>RetailSkillFormula.cs</c>'s doc
+    /// comment, corrected from this test's result.
+    /// </summary>
+    [InstalledDatFact]
+    public void SkillTable_MinLevelDistribution_NeverExceedsTrained()
+    {
+        using var dats = new DatCollection(DatDirectory, DatAccessType.Read);
+        DatReaderWriter.DBObjs.SkillTable? skillTable =
+            dats.Get<DatReaderWriter.DBObjs.SkillTable>(0x0E000004u);
+        Assert.NotNull(skillTable);
+
+        var byMinLevel = skillTable!.Skills.Values
+            .GroupBy(skill => skill.MinLevel)
+            .OrderBy(g => g.Key)
+            .ToDictionary(g => g.Key, g => g.Count());
+        Console.WriteLine(
+            "[CC5-R3-DAT] SkillTable MinLevel distribution (level=count): "
+            + string.Join(", ", byMinLevel.Select(p => $"{p.Key}={p.Value}"))
+            + $" (total skills: {skillTable.Skills.Count})");
+
+        // RetailSkillFormula.CalculateChargenScore's own gate argument ("the
+        // decomp's own `if (edi_1 >= MinLevel)` gate passes for both callers,
+        // Trained=2 and Specialized=3") only holds while every skill's
+        // MinLevel stays at or below Trained(2) — pin that as a real
+        // assertion instead of leaving it as prose, so a future DAT
+        // revision that adds a MinLevel=3+ skill fails HERE, not silently
+        // under-credits that skill's chargen score.
+        Assert.True(
+            byMinLevel.Keys.All(minLevel => minLevel <= 2),
+            "A skill's MinLevel exceeded 2 (Trained) in the installed DAT — "
+            + "RetailSkillFormula.CalculateChargenScore's Trained/Specialized "
+            + "gate argument needs re-verification for this skill.");
+    }
+
     private static void AssertButton(ImportedLayout layout, uint elementId) =>
         Assert.IsType<UiButton>(layout.FindElement(elementId));
 

tests/AcDream.App.Tests/UI/Layout/CharacterCreationUiControllerTests.cs

@@ -1055,6 +1055,51 @@ public sealed class CharacterCreationUiControllerTests
         Assert.False(field.CharacterFilter!('$'));
     }
 
+    /// <summary>
+    /// CC5 re-review residual round, R1 (2026-08-16): pins the F1 fix (the
+    /// deleted <c>_suppressNextFieldEvent</c> latch) against reintroduction.
+    /// The pre-fix bug needed BOTH halves to reproduce: (1) an EXTERNAL
+    /// change to <c>snapshot.Name</c> lands while the field is unfocused —
+    /// <c>CharacterCreationSummaryPage.Refresh</c>'s own field-sync block
+    /// (the F1 fix site) calls <c>field.SetText(...)</c> programmatically,
+    /// which armed the old latch — then (2) the PLAYER's own REAL commit
+    /// (<c>SetText</c> + <c>Submit</c>, the actual event path a keystroke +
+    /// Enter/blur drives — never a direct <c>CommitNameFromField</c> call)
+    /// arrives afterward. Pre-fix, that real commit hit the still-armed
+    /// latch and was silently dropped; <see cref="SummaryNameField_Submit_CommitsTheName"/>
+    /// alone never exercised this because it never drives step (1) first —
+    /// the review's own finding was that the claimed regression coverage
+    /// didn't actually touch the page.
+    /// </summary>
+    [Fact]
+    public void SummaryNameField_RealCommitAfterExternalRefreshWhileUnfocused_StillReachesSetName()
+    {
+        using var environment = new EnvironmentHarness();
+        environment.Controller.Open();
+        GoToSummary(environment);
+        UiField field = environment.SummaryNameField();
+
+        // (1) External change while unfocused: something OTHER than this
+        // field bumps the Runtime revision with a changed Name (e.g. the
+        // player edited another page and came back) — Refresh's field-sync
+        // block programmatically overwrites the field via SetText, which
+        // never raises OnFocusLost/OnSubmit.
+        RuntimeCharacterCreationSnapshot snapshot = environment.Runtime.View.Snapshot;
+        environment.Runtime.View.Snapshot = snapshot with
+        {
+            Revision = snapshot.Revision + 1,
+            Name = "Zorak",
+        };
+        environment.Controller.Tick();
+        Assert.Equal("Zorak", field.Text);
+
+        // (2) The player's own real commit afterward.
+        field.SetText("Adventurer");
+        field.Submit();
+
+        Assert.Equal("Adventurer", environment.Runtime.LastSetName);
+    }
+
     // ── CC5 review fix round F12(d): RebuildListbox content ─────────────
 
     /// <summary>