diff --git a/docs/research/2026-08-12-fa2-review-mechanism.md b/docs/research/2026-08-12-fa2-review-mechanism.md index a0f91e6c..0d51d777 100644 --- a/docs/research/2026-08-12-fa2-review-mechanism.md +++ b/docs/research/2026-08-12-fa2-review-mechanism.md @@ -476,3 +476,222 @@ build permitted for this review). | SF-4 | SHOULD-FIX | Locked/departed admission gate + `0x02BE` field 8 not modelled | | SF-5 | SHOULD-FIX | `AllegianceProfileLookups` duplicated in Runtime instead of reused | | SF-6 | SHOULD-FIX | Defaulted checkpoint snapshots carry null `Name`/`AllegianceName` | + +--- + +# 5. Narrow re-review — 2026-08-12 + +**Scope:** verify each finding's disposition in the actual fix-round diffs +(`4272ad0e`, `ded23067`, `ed8b3ec9`), spot-verify the blast dispositions, and +check the suite claim. Read-only. + +## VERDICT: CLOSED — no reopen. One carry-forward (CF-1) for FA5. + +All 2 MUST-FIX and all 6 SHOULD-FIX from this doc are correctly applied, and +every blast disposition I spot-checked holds. Nothing was skipped, and no fix +introduced a new mechanism defect. The two MUST-FIX corrections are real ports, +not suppressions. + +## 5.1 MUST-FIX dispositions + +**MF-1 — FIXED.** `RuntimeAllegianceState.ResetSession()` (`:192-209`) +delegates to a new `ClearLocked()` that clears the profile **and** drops +`_hasServerSeed`, exactly the `RuntimeCharacterOptionsState.ResetSession` +clear-and-relatch shape. `RuntimeGenerationResetStage.Allegiance = 13` is new +(`RuntimeGenerationReset.cs:39-58`), drained at +`case …Allegiance: Advance(state, _allegiance.ResetSession)` (`:320-322`), and +the owner is constructor-injected with a null check (`:113,148-149`). The enum +stays contiguous (`Allegiance = 13` … `Complete = 22`), which `Advance`'s +`state.Stage++` requires. **Stage enumerations are consistent everywhere:** a +whole-tree grep for `RuntimeGenerationResetStage.` outside its own file returns +only by-**name** references (`LiveSessionResetPlanTests.cs:279`, +`RuntimeGenerationResetTests.cs:121,160`, two doc-comment ``s) — no +consumer depends on an ordinal and nothing serializes it, so the +1 shift is +inert. The test is inverted, not deleted: +`FellowshipClearsAtResetButAllegianceSurvivesReconnect` → +`FellowshipAndAllegianceBothClearAtGenerationReset`, now asserting +`!HasProfile`, `!HasServerSeed`, `AllegianceName == string.Empty`, +`MonarchGuid == 0`, `RecordCount == 0`. + +One shape worth naming as correct rather than sloppy: `ResetSession` carries +**no** disposed guard, and `ClearLocked` on already-cleared fields computes +`changed == false` so it does not even bump the revision — a true no-op after +`Dispose`. That is right for a *retryable* reset transaction (a throwing guard +could never converge on retry), it matches the +`RuntimeInventoryState.ResetExternalContainer` / +`RuntimeCommunicationState.ResetNegotiatedChannels` precedent, and +`RuntimeFellowshipState.ResetSession` was brought to the same shape in +`ded23067` (blast SF-5). Note this is deliberately the opposite of SF-2's +lock-first *mutator* guard — mutators must reject post-dispose writes, reset +must tolerate them. Both are right. + +**MF-2 — FIXED.** `ApplyInfoResponseSelf` is deleted from +`RuntimeAllegianceState`; the `onAllegianceInfoResponseSelf` delegate hole is +deleted from `GameEventWiring.WireAll`; the `TargetGuid == playerGuid()` +self-gate is deleted from the `AllegianceInfoResponse` registration, which is +now the pre-FA2 three-line parse → null-check → `FormatAllegianceInfoLines` +chat emission and nothing else; the router's forwarding lambda is gone. A +whole-tree grep for `ApplyInfoResponseSelf|onAllegianceInfoResponseSelf` +returns **0 hits** in `src/` and `tests/`. The test was rewritten rather than +dropped: `WireAll_AllegianceInfoResponse_SelfGated_FiresOnlyForOwnGuid` → +`…_IsTextOnly_ForSelfAndOtherGuidsAlike`, pinning the chat output firing +unconditionally for both a self query and a by-name query. The stale-`Rank` +second-order defect dies with the seeding path. + +## 5.2 SHOULD-FIX dispositions + +**SF-1 — FIXED.** All nine delegate holes are now passed conditionally +(`social.Fellowship is { } x ? x.ApplyFullUpdate : null`, etc., +`LiveSessionEventRouter.cs`), so `GameEventWiring` registers a handler only +when an owner exists and `GameEventDispatcher.GetUnhandledCount` reports +correctly for owner-less callers. Nine, not ten — `onAllegianceInfoResponseSelf` +was removed by MF-2, which is consistent. + +**SF-2 — FIXED.** Every mutator in both owners now takes `_gate` first and +checks the plain `_disposed` field inside it +(`RuntimeFellowshipState.ApplyFullUpdate`/`ApplyUpdateFellow`/`ApplyQuit`/ +`ApplyDismiss`/`ApplyDisband`; `RuntimeAllegianceState.ApplyUpdate`/ +`ApplyLoginNotification`/`ApplyUpdateDone`/`ApplyUpdateAborted`), matching +`RuntimeCommunicationState.cs:377-379`. The decode-thread-vs-host-thread window +that could repopulate a disposed owner and permanently falsify +`CaptureOwnership().IsConverged` is closed. + +**SF-3 — FIXED, and the port is faithful.** `RecalculateEvenXpSplit()` is a +line-for-line port of lane B §2.10's transcription of +`Fellowship::RecalculateEvenXPSplitting @0x005B92E0`: early-return on +`!_shareXp` leaving the flag untouched; min/max scan; `_evenXpSplit = true`; +then `if (minLevel < 50) { if (maxLevel > leaderLevel + 5) false; +if (minLevel + 5 < leaderLevel) false; }`. Called from `ApplyUpdateFellow` +after the upsert (retail: `Fellow::operator=` then +`RecalculateEvenXPSplitting`, `@0x005B977E`/`@0x005B9785`) and from +`ApplyQuit`/`ApplyDismiss` on the removal branch only (retail `RemoveFellow +@0x005B96EE`) — **not** from `ApplyFullUpdate`, which is correct and is the +part that could easily have been got wrong: retail's `Fellowship::operator=` +copies the server's authoritative flag and does not recompute. `ClearLocked` +does not recompute either, which is right — a full clear is retail's +`DeleteFellowship`, not `RemoveFellow`. + +Two deliberate departures, both flagged in the code and both defensible: +`minLevel` seeds from `uint.MaxValue` rather than retail's `100000` (identical +for any reachable level, and the empty-table case converges to the same +result); and the leaderless-table case returns `_evenXpSplit = true` instead of +replaying retail's unsigned wraparound against the `0xFFFFFFFF` sentinel. I +verified that second one against the cited source rather than trusting the +comment: lane B §7.4 (`2026-08-11-fa-fellowship-wire.md:962-966`) says verbatim +*"treat a leaderless table as 'leave `_even_xp_split` at 1'"*. The citation is +accurate and the lane is the pinned spec. + +**SF-4 — FIXED.** `ApplyFullUpdate` now stores `update.Departed` into a +`_fellowsDeparted` dictionary (cleared alongside `_members` in both +`ApplyFullUpdate` and `ClearLocked`), and `ApplyUpdateFellow` gates the +new-guid branch through `IsAdmissibleWhileLocked` — `_fellowsDeparted` +membership plus a 900 s (`0x384`) window, matching `Fellowship::AddFellow +@0x005B94A5`. The gate correctly applies **only** to the "is this guid NEW" +branch; an existing member's refresh is never gated, matching retail's +`UpdateFellow`-hits-the-table path, and a refused guid skips the recompute +exactly as retail's early-returning `AddFellow` does. I confirmed the gate has +real data to work with: FA1's parser does read `0x02BE` field 8 +(`GameEvents.cs:693-701` — `[u16 count][u16 buckets]` then +`count × [u32 guid][i32 timestamp]`), so this is not an always-refuse gate. + +**SF-5 — FIXED.** `AllegianceProfileLookups` promoted `private` → `internal` +with `` added to +`AcDream.Core.Net.csproj`; `TryGetMember`/`TryGetPatron`/`GetVassals` now call +`FindData`/`FindPatron`/`FindVassals` and the duplicated `TryGetMemberLocked` +helper is deleted. One implementation of the retail walk again. + +**SF-6 — FIXED.** `RuntimeStateCheckpoint`'s `Fellowship`/`Allegiance` +parameters are now required rather than `= default`, and both snapshot types +gained an explicit parameterless constructor seeding `string.Empty`. The stated +reasoning is correct C#: `default(T)` bypasses a struct's parameterless +constructor, and CS1736 forbids a non-constant `new(...)` as an optional +parameter default — so required parameters really were the only way to +guarantee a non-null `Name`/`AllegianceName`. + +## 5.3 Blast dispositions (spot-verified) + +- **Teardown ledger (blast MF-1) — FIXED, better than the report asked for.** + Case 9 was genuinely one flag too many (it included `FellowshipDisposed`, the + stage it had not yet run); it is now + `Complete & ~Fellowship & ~Allegiance & ~Identity & ~EntityObjects` = 9 + flags. I re-derived the whole table: at `_disposeStage == N` exactly N flags + are set, for every N in 0..13. The new + `CompletedTeardownStagesAccumulatesExactlyOneFlagPerStage` reflection walk + pins **every** intermediate stage (the previous tests only sampled stage 0 + and `Complete`, which is precisely why the off-by-one survived), and its + closing `Assert.Equal(Complete, expected)` ties the hand-written ordered flag + list back to the enum so a future owner insertion cannot silently drop or + duplicate a flag. +- **Router self-guid test (blast SF-4) — FIXED.** + `FellowshipQuit_RoutesSelfGuidToClearAndOtherGuidToRemove` builds real + `RuntimeFellowshipState`/`RuntimeAllegianceState` owners, wires them through + the one production `LiveSessionEventRouter` registration site with + `PlayerGuid: () => self`, and dispatches real `0x00A3` envelopes for both an + other-quit (one member removed, still in fellowship) and a self-quit (whole + snapshot cleared). This was the slice's only non-trivial lambda and it now + has end-to-end coverage. +- **`ResetSession` guard alignment (blast SF-5) — FIXED**, see MF-1 above. +- **`GetVassals` allocation (blast SF-7) — documented** as an intentional + exception with the reason (`yield return` is illegal inside a `lock`) and a + caching hint for FA3. Acceptable disposition for a convention note. +- **Seam-doc addenda (blast SF-1/SF-2) — LANDED and dated.** §1.3's inverted + `RuntimeCharacterOptionsState` citation, §2.3/§9's "dispatcher permits + multiple owned handlers", the §8 table's Allegiance row, and the + executive-summary ownership bullet all carry + `[FA2 fix-round addendum, 2026-08-12 …]` strikes citing the review docs; + §2.3 additionally gains the `0x01C9`/`0x01CA` "correctly left unregistered" + disposition it was missing. The plan's D2, its architecture blurb, and FA2's + slice-map contract row all carry matching dated corrections, so the + "allegiance survives reconnect" claim does not survive uncorrected anywhere. +- **Ledger count corrections (blast SF-3) — LANDED:** 11 `Send*` wrappers (not + 12 — `SendAllegianceInfoRequest` pre-dates FA2) and 10 delegate holes (not + 15). I re-counted both against the diffs; both corrected figures are right. +- **Register-row re-evaluation (blast SF-6) — I audited the conclusion and + AGREE.** With MF-1's clear-at-reset landed, acdream's allegiance lifetime now + *matches* `ClientAllegianceSystem::OnEndCharacterSession @0x00569FA0` → + `AllegianceProfile::Clear`, and with MF-2 landed `0x0020 AllegianceUpdate` is + the sole seeder, matching retail's sole `AllegianceProfile::operator=` writer + at `@0x0056A129`. The register tracks acdream-vs-retail *deviation*; there is + no deviation left for a row to name — the same reasoning FA1's D9 addendum + used for the absent WeenieError strings. **No register row is owed, and no + other FA2 deviation surfaced during this re-review.** TS-81 (login-notice + text) and TS-80 (narrowed) remain correct as filed. + +## 5.4 Suite claim + +**Consistent with the diffs, exactly.** Counting discovered xunit cases +(`[Fact]` + `[InlineData]` rows) at `12053e61` vs the fix-round tip: +`RuntimeAllegianceStateTests` 7 → 9 (**+2**: one `ApplyInfoResponseSelf_…` +deleted, three `ResetSession_*` added); `RuntimeFellowshipStateTests` +10 Facts + 4 InlineData = 14 → 17 Facts + 7 InlineData = 24 (**+10**); +`GameRuntimeTests` **+1**; `LiveSessionEventRouterTests` **+1**; +`RuntimeGenerationResetTests` and `GameEventWiringTests` **+0** (one in-place +rename each). Total **+14**, matching the ledger's 13,201/4/0 → 13,215/4/0 +claim. The absolute totals were not independently re-measured (no build +permitted); the delta is exact. + +Targeted runs on the post-fix Release binaries (built 02:19, after the last +source edit at 02:06 and the final code commit at 02:17): +`AcDream.Runtime.Tests` filtered to Fellowship/Allegiance/GenerationReset/ +GameRuntimeTests/LiveSessionEventRouter — **95 passed, 0 failed, 0 skipped**; +`AcDream.Core.Net.Tests` filtered to GameEventWiring — **67 passed, 0 failed, +0 skipped**. + +## 5.5 Carry-forward (FA5, not a reopen) + +**CF-1 — nothing re-subscribes allegiance now that the reset clears it, and the +plan's FA5 row names the wrong opcode for the panel-show path.** Retail sends +`Event_UpdateRequest(1)` (`0x001F`) at `gmAllegianceUI::PostInit @0x004911C6`, +at `RecvNotice_PlayerDescReceived @0x00490D59` (i.e. every character-session +start), and on the panel's visible branch `@0x004912DD` (lane C §1.2). acdream +wires `0x001F` to no lifecycle event, so post-reset the owner stays empty until +ACE happens to push `0x0020` on a tree change. Before MF-1 this was masked by +persistence; the fix correctly exposes it. No register row is owed while there +is no allegiance panel at all (the whole surface is FA3/FA5 scope) and no +consumer reads the owner, but FA5 must wire it — and the plan's FA5 row +currently says *"profile parse on show (`0x027B`)"*, which is the by-name +**info request**, not the subscribe toggle. That distinction is now +load-bearing: after MF-2, `0x027B`'s response (`0x027C`) is text-only and no +longer feeds the owner at all, so an FA5 that follows the plan literally would +open a panel with chat text and no data. Correct the plan row to `0x001F` +before FA5 starts.