acdream/docs/research/2026-08-12-fa2-review-mechanism.md
Erik cc1a319c1f docs: FA2 mechanism re-review -- CLOSED, no reopen (1 carry-forward)
All 2 MUST-FIX and all 6 SHOULD-FIX dispositions re-derived in the actual
fix-round diffs (4272ad0e, ded23067, ed8b3ec9); every blast disposition
spot-verified. Nothing skipped, no fix introduced a new mechanism defect.

MF-1: RuntimeGenerationResetStage.Allegiance added and drained, the owner
clears the profile AND drops HasServerSeed, the inverted test flipped. Stage
enumeration verified consistent everywhere -- every reference outside the
enum's own file is by NAME, nothing serializes the ordinal, so the +1 shift
is inert. MF-2: ApplyInfoResponseSelf, the delegate hole and the self-gate
are all gone (0 whole-tree hits); the test was rewritten to pin text-only
output for self and other guids alike.

SF-3's RecalculateEvenXPSplitting port checked line-for-line against lane B
2.10, including the deliberate leaderless-table departure -- lane B 7.4 says
verbatim "treat a leaderless table as leave _even_xp_split at 1", so the
citation is accurate. SF-4's 900s gate confirmed to have real data (FA1 does
parse 0x02BE field 8) and to gate only the new-guid branch. SF-1/2/5/6 all
land as specified.

Blast: the teardown table re-derived for every N in 0..13 (case 9 was
genuinely one flag over); the new reflection walk pins every intermediate
stage; seam-doc and plan addenda are dated and accurate; the corrected 11/10
counts are right. Audited blast SF-6's no-register-row conclusion and AGREE
-- clear-at-reset plus 0x0020-only seeding means acdream now matches retail,
so no deviation remains for a row to name.

Suite claim 13,201/4/0 -> 13,215/4/0 (+14) reproduced exactly by counting
discovered cases per file. Targeted post-fix Release runs: 95/95 Runtime,
67/67 Core.Net.

CF-1 (FA5, not a reopen): nothing re-subscribes 0x001F now that the reset
clears the owner, and the plan's FA5 row cites 0x027B for the panel-show
path -- which after MF-2 is text-only and feeds nothing.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-08-12 02:30:28 +02:00

39 KiB
Raw Permalink Blame History

Campaign FA slice FA2 — mechanism-faithfulness review

Reviewer lens: mechanism faithfulness (retail behavior vs. shipped code). Target: 1c401048 (Core.Net), 369729f0 (Runtime), cced83b4 (App/Headless), 12053e61 (docs/ledger), against docs/plans/2026-08-11-fellowship-allegiance-campaign.md slice FA2 + D2/D4, lanes B/C/D (2026-08-11-fa-fellowship-wire.md, 2026-08-11-fa-allegiance-wire.md, 2026-08-11-fa-acdream-seams.md incl. its FA1 fix-round addenda), and the J4.x precedent closeouts. Date: 2026-08-12.

Verdict

APPROVE-WITH-FIXES — 2 MUST-FIX, 6 SHOULD-FIX.

Both MUST-FIX findings are on the allegiance owner and they compound: the owner is seeded from a message retail never seeds from (MF-2), and it is never cleared at the boundary retail clears it at (MF-1). Every fellowship mechanism in the slice re-derived clean against the decomp. Nothing in the wiring, command, view, teardown, or register work is wrong.

Neither MUST-FIX is user-visible today — no FA2 command or view has a production consumer yet (verified below). They are must-fix because FA3FA5 build the panel directly on this owner, and MF-1 is already pinned by a passing test, which is the "wrong behavior locked in by test" shape FA1's own review called out when it deleted AllegianceTree.


1. MUST-FIX

MF-1 — Allegiance surviving a generation reset contradicts retail, contradicts its own cited precedent, and is pinned by a test

RuntimeAllegianceState participates in no reset stage (src/AcDream.Runtime/RuntimeGenerationReset.cs:39-41 says so explicitly; src/AcDream.Runtime/Gameplay/RuntimeAllegianceState.cs:18-27 documents the intent), and tests/AcDream.Runtime.Tests/RuntimeGenerationResetTests.cs (FellowshipClearsAtResetButAllegianceSurvivesReconnect) asserts the profile, the name and HasServerSeed all survive ResetGeneration.

(a) Retail clears it at exactly this boundary.

00569fa0  void __fastcall ClientAllegianceSystem::OnEndCharacterSession(...)
00569fa3      return AllegianceProfile::Clear(&this->m_allegianceProfile);

(docs/research/named-retail/acclient_2013_pseudo_c.txt:375000-375004, symbols.json 0x00569FA0.) AllegianceProfile::Clear @0x005B6A60 (:453700-453707) zeroes _total_members, _total_vassals and tail-calls AllegianceHierarchy::Clear — the whole tree.

The sibling hook is the control that proves the boundary is the right one:

005690a0  void __fastcall ClientFellowshipSystem::OnEndCharacterSession(...)
005690a8      if (m_pFellowship != 0) m_pFellowship->vtable->__vecDelDtor(1);
005690b0      this->m_pFellowship = nullptr;

(:373778-373790.) FA2's fellowship half is byte-faithful; the allegiance half is the exact inverse of what retail does at the same hook.

The hook is a per-character-session pair, and it maps 1:1 onto acdream's generation: UIQueueManager::OnBeginCharacterSession fires on PlayerDescription (0x13, @0x0055B173, :359247) and UIQueueManager::OnEndCharacterSession fires from gmGamePlayUI::~gmGamePlayUI (@0x004EA339, :238072), fanning out to every client system's vtable slot (UIQueueManager::OnEndCharacterSession @0x0055A6D0, :358492-358501).

Corroboration that the cached profile really is per-session state: gmAllegianceUI::RecvNotice_AllegianceLogin @0x00492220 (:158415-158426) gates every logon/logoff line on AllegianceProfile::GetData(&eax->m_allegianceProfile, guid, …) — the cached profile, i.e. the thing Clear empties.

(b) The cited precedent CLEARS and re-latches — the citation is inverted.

RuntimeAllegianceState.cs:24-27 and lane D §1.3 (2026-08-11-fa-acdream-seams.md:144-148) both justify persistence as "same class as RuntimeCharacterOptionsState's HasServerSeed latch". That precedent does the opposite:

// src/AcDream.Runtime/Gameplay/RuntimeCharacterState.cs:1007-1017
public void ResetSession()
{
    Volatile.Write(ref _options1, DefaultOptions1);
    Volatile.Write(ref _options2, DefaultOptions2);
    Interlocked.Increment(ref _revision);
    lock (_dirtyGate) { _isDirty = false; _hasServerSeed = false; }
}

and its own doc comment (:999-1006) names the hazard FA2 is now exposed to verbatim: "a stale seed surviving a session boundary could let a flush ship the PRIOR character's words over the new one's." It runs every generation via reset stage Character = 7RuntimeCharacterState.ResetSessionTry(Options.ResetSession, …) (RuntimeCharacterState.cs:442).

(c) The process model does NOT preclude a cross-character reset.

The graphical host constructs its connect options with no character selector:

// src/AcDream.App/Composition/SessionPlayerComposition.cs:1127-1132
new LiveSessionConnectOptions(
    d.Options.LiveMode, d.Options.LiveHost, d.Options.LivePort,
    d.Options.LiveUser ?? string.Empty, d.Options.LivePass ?? string.Empty)

LiveSessionConnectOptions.Character therefore defaults to null (src/AcDream.Runtime/Session/LiveSessionContracts.cs:16), so LiveSessionController.StartCore (:611-618) falls through TrySelectCharacter (:841-852) to CharacterList.TrySelectFirstAvailable, whose only filter is IsAvailableActiveIdentity(c) => c.Id != 0 && c.SecondsGreyedOut == 0 (src/AcDream.Core.Net/Messages/CharacterList.cs:111-112) — and ACE derives secondsGreyedOut from character.DeleteTime (references/ACE/.../GameMessageCharacterList.cs:28). Which character enters world at generation N+1 is resolved fresh from a server-supplied list, and nothing in RuntimeAllegianceState keys on character identity. The reset genuinely runs between the two: StopCore (LiveSessionController.cs:676-701) always drains the retired scope, whose teardown calls Host.ResetSessionState(Generation) (:231), which is the graphical host's _domain.Runtime.ResetGeneration(...) hop (src/AcDream.App/Net/LiveSessionRuntimeFactory.cs:193).

Headless is not exposed: HeadlessSessionHost requires exactly one selector field (MapCharacterSelector, :830-835; a fully-null selector makes selectorCount != 1 and TrySelectCharacter returns false), so each headless root is character-pinned for its whole life. The exposure is the graphical host only — but the graphical host is the one FA3FA5 target.

(d) There is no re-seed that would mask it.

ACE pushes 0x0020 AllegianceUpdate on a tree change (AllegianceManager.cs:398, Allegiance.cs:361) or in reply to 0x001F (GameActionAllegianceUpdateRequest.cs:9-22). FA2 wires no 0x001F sender to any lifecycle event (verified in SF/clean §3.4). Retail's own re-seed is gmAllegianceUI::RecvNotice_PlayerDescReceived @0x00490D59Event_UpdateRequest(1) (lane C §1.2), which acdream does not have yet. So for a second character with no allegiance, character A's AllegianceName, monarch, vassal list, HasProfile and HasServerSeed stay readable indefinitely.

Recommended fix. Add RuntimeGenerationResetStage.Allegiance calling a new RuntimeAllegianceState.ResetSession() that clears the profile and _hasServerSeed (mirroring the options precedent exactly); invert the test; correct plan §2 D2 and lane D §1.3's inverted precedent citation. Anchor the missing PlayerDesc-triggered 0x001F(1) resubscribe to FA5 as a TS row so the clear does not silently mean "blank until the panel is opened". If the campaign still wants persistence after seeing this, it needs BOTH a divergence-register row (CLAUDE.md: same commit as the deviation) AND a character-identity key on the retained data — persistence with no identity guard is the failure mode, not the persistence itself.

MF-2 — 0x027C AllegianceInfoResponse must not seed the cached profile; retail's handler is print-only

RuntimeAllegianceState.ApplyInfoResponseSelf (RuntimeAllegianceState.cs:101-116) writes monarch, records, name, totals, _hasProfile and _hasServerSeed from 0x027C. Retail does not.

The dispatcher unpacks 0x027C into a stack-local profile and destroys it at return:

006a7470  CM_Allegiance::DispatchUI_AllegianceInfoResponseEvent(...)
006a74cd      void var_118;  CAllegianceProfile::CAllegianceProfile(&var_118);
006a74f1      AllegianceProfile::UnPack(&var_118, &arg2, …);
006a74ff      ClientAllegianceSystem::Handle_Allegiance__AllegianceInfoResponseEvent(
                  arg1->m_pAllegianceSystem, edi, &var_118);
006a750a      AllegianceHierarchy::~AllegianceHierarchy(&var_114);

(acclient_2013_pseudo_c.txt:686294-686313.) The handler (@0x0056A1D0, :375144-375210) reads only arg3 and calls ClientSystem::AddTextToScrollit never touches this->m_allegianceProfile.

Contrast the only path that does seed:

0056a120  ClientAllegianceSystem::Handle_Allegiance__AllegianceUpdate(...)
0056a129      AllegianceProfile::operator=(&this->m_allegianceProfile.vtable, arg2);

(:375103-375109.) 0x0020 is the single inbound writer of the cached profile, which is why lane C describes 0x0020 as the subscribed push and 0x027C as a query response.

Second-order defect the divergence already carries. 0x027C has no rank field, so ApplyInfoResponseSelf deliberately retains the previous _rank (RuntimeAllegianceState.cs:97-99). On a client where the only allegiance message ever received is a self @allegiance info response, Snapshot.HasProfile == true with Snapshot.Rank == 0 — FA5's monarch/self block would render a ranked character as rank 0 (no title). That state is unreachable in retail precisely because retail never seeds from 0x027C.

Recommended fix. Drop ApplyInfoResponseSelf, its onAllegianceInfoResponseSelf delegate hole, and the self-gate, leaving the @allegiance info chat path exactly as it is today (the fold itself is correct — see §3.2). If it is kept deliberately, it needs a register row naming the rank hazard.


2. SHOULD-FIX

SF-1 — The nine new delegate holes are passed unconditionally, so nine event types silently leave the unhandled-count diagnostic. LiveSessionEventRouter.cs:238-264 passes non-null lambdas (update => social.Fellowship?.ApplyFullUpdate(update) etc.) regardless of whether social.Fellowship/Allegiance is supplied, so GameEventWiring's if (onX is not null) guards always pass and the handlers always register. GameEventDispatcher.Dispatch (:95-117) only increments _unhandledCounts when there is no registration, so GetUnhandledCount(FellowshipFullUpdate) and its eight siblings now read 0 for any caller that did not supply an owner (bare-ChatLog tests, future partial hosts) even though the parse result is discarded. Pass the delegates conditionally, or hoist the null check to the binding site.

SF-2 — Disposed check is outside the lock in both new owners (TOCTOU). Every mutator does ObjectDisposedException.ThrowIf(IsDisposed, this) and then takes _gate separately (RuntimeFellowshipState.cs:69-71, 94-96, 112-114, 133-135, 148-150, 207-208; RuntimeAllegianceState.cs:80-82, 104-106, 132-134, 139-141, 146-148). IsDisposed takes and releases the lock itself, so an inbound event racing Dispose can repopulate _members/_records after _disposed = true, permanently falsifying CaptureOwnership().IsConverged and failing the J-slice ownership ledger at teardown. The established precedent checks inside the lock (RuntimeCommunicationState.cs:377-379). Inbound events run on the decode thread and Dispose on the host thread, so this is a real window, currently masked by the router's accepting gate.

SF-3 — Retail's client-side RecalculateEvenXPSplitting is not modelled. Fellowship::UpdateFellow @0x005B9730 calls Fellowship::RecalculateEvenXPSplitting(this) at @0x005B9785 on every upsert (:457071-457098), as do AddFellow @0x005B94D5 and RemoveFellow @0x005B96EE (lane B §2.10). FA2 stores _evenXpSplit from the full update and never recomputes it on ApplyUpdateFellow/ApplyQuit/ ApplyDismiss. FA4's share-percentage column branches on exactly this field (lane B §2.9). Against ACE it is largely masked — RemoveFellowshipMember and AddFellowshipMember both finish with UpdateAllMembers(), a FullUpdate to everyone (references/ACE/Source/ACE.Server/Entity/Fellowship.cs:179,219-227) — but the retail mechanism is absent, and the owner is where retail keeps it.

SF-4 — Retail's locked/departed admission gate on the upsert path is not modelled. Fellowship::UpdateFellow falls through to Fellowship::AddFellow @0x005B9480 when the guid is absent (@0x005B976F), and AddFellow refuses a guid while _locked unless it is in _fellows_departed within 900 s (lane B §2.11, @0x005B94A5). FA2's ApplyUpdateFellow always inserts, and ApplyFullUpdate discards update.Departed entirely (RuntimeFellowshipState.cs:67-84 — field 8 of 0x02BE per lane B §3.9). The insert-a-new-member branch IS live against ACE (AddFellowshipMember sends UpdateFellow to existing members before the FullUpdate, Fellowship.cs:161-163), so this is reachable, not theoretical.

SF-5 — AllegianceProfileLookups is re-implemented rather than reused. RuntimeAllegianceState's TryGetMember/TryGetPatron/GetVassals (:214-290) duplicate FA1's ClientCommandResponses.AllegianceProfileLookups (:250-294) line for line. They agree today — I diffed both against AllegianceHierarchy::GetPatron @0x005B6DD0 (:453991-454011) and the §4.4 reversal rule and both are correct — but two copies of a retail walk is the shape that drifts. AllegianceProfileLookups is private static; promoting it to internal (or public) lets Runtime call the one implementation.

SF-6 — RuntimeStateCheckpoint's new defaulted snapshots carry null strings. GameRuntimeViews.cs:241-245 defaults Fellowship/Allegiance to default, whose Name/AllegianceName are null (not string.Empty — the owners' own initialisers are correct, RuntimeFellowshipState.cs:41, RuntimeAllegianceState.cs:49). Only test-constructed checkpoints hit it (GameRuntimeContractTests.cs:150 is the sole positional site), and the trace recorder only reads scalars (GameRuntimeEvents.cs:184-189), so nothing NREs today. Worth a non-null default.

Observations (no action requested). RuntimeFellowMemberSnapshot.ShareLoot is exposed as bool while lane B §4.1's recommendation is "model as a raw uint" — the predicate is right (!= 0, never == 1, RuntimeFellowshipState.cs:291-293, pinned by ShareLoot_ReadsTheRawWireBitAsNonZero_NeverEqualsOne) and the raw value is retained in the owner, so this is a view-shape preference, not a defect. IRuntimeFellowshipView has no member enumeration (only TryGetMember(guid) + MemberCount), which FA4's roster list box will need.


3. What verified clean

3.1 Fellowship lifecycle rules — every rule re-derived against the decomp

Rule FA2 Retail
Full update REPLACES ApplyFullUpdate clears then refills, sets _isInFellowship (:67-84) Handle_Fellowship__FullUpdate @0x005693C0: allocate CFellowship if absent, then Fellowship::operator= (:374037-374071)
Incremental UPSERTS by guid _members[update.MemberGuid] = update.Member (:92-101) Fellowship::UpdateFellow @0x005B9730Fellow::operator= on hit, AddFellow on miss (:457071-457098)
SELF quit clears the WHOLE snapshot ApplyQuit, quitterGuid == selfGuidClearLocked() (:110-124) Handle_Fellowship__Quit @0x00569E90: arg2 != player_idRemoveFellow, else DeleteFellowship (:374914-374950)
Other-quit removes ONE _members.Remove(quitterGuid) same site
Dismiss identical to quit ApplyDismiss (:130-144) Handle_Fellowship__Dismiss @0x00569F10 is byte-identical (:374953-374985)
Disband always clears ApplyDisband (:147-151) Handle_Fellowship__Disband @0x00569E70DeleteFellowship (:374903-374910)

The if (!_isInFellowship) return; guards on the three incremental mutators are strictly defensive: retail passes a possibly-null m_pFellowship straight into Fellowship::IsFellow/RemoveFellow with no guard, so the equivalent retail state faults rather than doing something different. ACE never reaches it (Player.LogOut_InnerFellowshipQuit(false), references/ACE/.../WorldObjects/Player.cs:531-532, so a reconnect is never mid-fellowship; and HandleFellowshipUpdateRequest sends a FullUpdate before any vitals stream, Player_Fellowship.cs:142-149).

The leader hand-off rule is exact. RequiresLeaderHandoffBeforeQuit (RuntimeFellowshipState.cs:172-193) returns true only when !disband && _isInFellowship && _leaderGuid == selfGuid and a non-self member exists. Retail's Quit button, gmFellowshipUI::ListenToElementMessage case 8 (:156425-156444):

0049034b  case 8:
00490353      if (m_pFellowship_1 != 0)
0049035d          if (m_pFellowship_1->_leader == GetPlayerID())
00490361              eax_18 = Fellowship::GetNonLeaderFellowID(m_pFellowship_1);
00490368              if (eax_18 != 0) AssignLeadershipToFellow(this, eax_18);
004903f1      CM_Fellowship::Event_Quit(0);

and case 0xC (Disband) is CM_Fellowship::Event_Quit(1) with no hand-off (:156475-156479). FA2 collapses the two retail buttons into one Quit(disband) and gates the hand-off on !disband — behaviourally identical. Fellowship::GetNonLeaderFellowID @0x005B9200 (:456723-456770) walks the hash table returning the first key != _leader; FA2 skips selfGuid, which equals _leaderGuid on that branch. Equivalent modulo iteration order, which retail does not define either.

Ordering is preserved on both hosts. Direct sends 0x0290 then 0x00A3 inline (DirectGameRuntimeCommandAdapter.cs Quit); App publishes two bus messages under one lock (LiveSessionCommandRouter.Publish, :293-300), so FellowshipAssignNewLeaderRuntimeCmd cannot be reordered after FellowshipQuitRuntimeCmd.

IsFull >= 9 — verified by absence. No FA2 code encodes a fellowship capacity anywhere (grep for IsFull returns only unrelated IsFullyConstrained/OutsideIsFullScreen/IsFullyResident), and RuntimeFellowshipSnapshot exposes MemberCount without an IsFull. Lane B §7.1's byte-decoded cmp [ecx+0x14],9 / sbb / inc result has nothing to be wrong about yet; FA4's UpdateButtons port is where it lands.

3.2 The shared 0x027C registration — the folding claim is correct and the live output is byte-identical

GameEventDispatcher.Dispatch (:95-117) is a single _handlers.TryGetValueregistration.Handler(envelope); Register (:57-76) writes _handlers[type] = node, i.e. it REPLACES. The seam doc §2.3's "both fire" claim is wrong and the implementer's correction is right — a literal second registrar.Register(AllegianceInfoResponse, …) would have silently killed the live @allegiance info output.

The folded handler (GameEventWiring.cs, AllegianceInfoResponse registration) keeps the pre-existing three lines verbatim — parse, if (info is null) return;, foreach (FormatAllegianceInfoLines) chat.OnSystemMessage(line, chatType: 0u) — and only appends the self-gated callback afterwards. One parse feeds both; there is no double-parse divergence and no reordering of the chat emission. Pinned by WireAll_AllegianceInfoResponse_SelfGated_FiresOnlyForOwnGuid, which asserts chatLines > 0 on both the self and the other-player response and observed == null only on the latter. (Note this verdict is about the fold; whether the callback should exist at all is MF-2.)

Minor: the gate is playerGuid is not null && info.Value.TargetGuid == playerGuid(), with no playerGuid() != 0 clause. ACE always writes a real guid (GameEventAllegianceInfoResponse.cs:9-11), so it is unreachable, but a != 0 clause would be free.

3.3 D4's 0x00A6 — present, generation-gated, not fired

IRuntimeFellowshipCommands.SetPanelOpen (GameRuntimeCommands.cs:341-343) is implemented on both adapters (DirectGameRuntimeCommandAdapter.cs:941, CurrentGameRuntimeCommandAdapter.cs:934), both behind the generation Validate gate, and reaches WorldSession.SendFellowshipUpdateRequestSocialActions.BuildFellowshipUpdateRequest. A whole-src grep for SetPanelOpen finds no caller other than the unrelated ToolbarController.SetPanelOpen(uint panelId, bool open). No premature send. The same holds for IRuntimeAllegianceCommands.SetUpdateSubscription (0x001F) — declared, implemented, uncalled.

3.4 Wiring symmetry

Both LiveSocialSessionBindings construction sites are updated in the same commit with identical named arguments — src/AcDream.App/Net/LiveSessionRuntimeFactory.cs:261-263 (Fellowship: _domain.Runtime.FellowshipOwner, Allegiance: _domain.Runtime.AllegianceOwner) and src/AcDream.Headless/Hosting/HeadlessSessionHost.cs:785-787 (Fellowship: Runtime.FellowshipOwner, Allegiance: Runtime.AllegianceOwner). There is still exactly one GameEventWiring.WireAll call site (LiveSessionEventRouter.cs:234), so both hosts get the same registration.

The self-guid used for the quit/dismiss self-vs-other split resolves to the same owner on every path: the router uses inventory.PlayerGuid(), bound to () => _player.Identity.ServerGuid in App (LiveSessionRuntimeFactory.cs:277) and () => Runtime.PlayerIdentity.ServerGuid in Headless (HeadlessSessionHost.cs:745); Direct uses _runtime.PlayerIdentity.ServerGuid; App's adapter uses _view.Lifecycle.PlayerGuid, which GameRuntime fills from PlayerIdentity.ServerGuid (GameRuntime.cs:484-488). One source.

The two adapters' zero-guid rejection paths look different (EmitUnsupported(...,Rejected,guid) vs EmitResult(...,Rejected,guid)) but are semantically identical — both call EventSink.EmitCommand(domain, op, status, primaryObjectId) and return Result(status, primaryObjectId); EmitResult merely also passes text: null (DirectGameRuntimeCommandAdapter.cs:1355-1383).

IRuntimeEventObserver is untouched — the 369729f0 diff of GameRuntimeEvents.cs adds only two RuntimeCommandDomain ordinals and six trace-string lines; the interface (:100-118) is unchanged, so the five bot policies and the trace recorder still compile. This honours D2.

The Runtime-only assembly guard still holds: GameRuntimeTests.cs:174-181 asserts no AcDream.App/Silk.NET assembly loads, and both new owners live in AcDream.Runtime depending only on AcDream.Core.Net.Messages types. GameRuntimeContractTests.cs gained both new types to its owned-type list.

3.5 TS-81

The register row is honest and the mechanism claim is verifiable. At gmAllegianceUI::RecvNotice_AllegianceLogin @0x00492220 the two candidate strings really do resolve through mislabelled vtable-slot symbols:

00492278      if (arg3 == 0)
00492281          var_48_1 = &gmAllegianceUI::`vftable'.RecvNotice_PrevSpellTab;
00492278      else
0049227a          var_48_1 = &gmAllegianceUI::`vftable'.RecvNotice_UpdateSpellComponents;
00492286      AC1Legacy::PStringBase<char>::PStringBase<char>(&arg3, var_48_1);

(:158434-158439) — a PStringBase<char> constructed from what BN typed as two unrelated vtable entries. Refusing to invent English here is correct. ApplyLoginNotification does bump the revision (RuntimeAllegianceState.cs:131-135), pinned by Revision_IsMonotonicAcrossEveryEventKind. TS-80's narrowing is also accurate: Create(gen, name, shareXp) takes the flag but no caller reads FellowshipShareXP into it.

3.6 Snapshot immutability, revision monotonicity, J-owner template

Snapshots are readonly record structs of scalars plus member value-copies; the parsed IReadOnlyList<AllegianceMemberRecord> stored by ApplyUpdate/ApplyInfoResponseSelf is a freshly allocated list per parse (ClientCommandResponses.ReadAllegianceProfileBody, :518-519) held by no one else, so there is no aliasing path back into the owner. Every mutator bumps a monotonic long; the no-change paths (ClearLocked when already empty, Remove returning false) deliberately do not, which is correct for a poll contract.

The 8-edit template is complete and the teardown maths is right: GameRuntimeTeardownStage renumbers IdentityDisposed 1<<9→1<<11 and EntityObjectsDisposed 1<<10→1<<12 (in-memory flags only — no serialized consumer), TeardownStageCount 11→13, and the stage masks at GameRuntime.cs:716-735 compose correctly, with 10/11/12 expressed as Complete & ~… of exactly the not-yet-run stages. RuntimeGenerationResetStage stays contiguous (Fellowship = 12 before BeginEntityRetirement = 13), which Advance's state.Stage++ requires. Both fault-injection construction points were added and are covered.

3.7 Test run

dotnet test tests/AcDream.Runtime.Tests --no-build -c Release --filter "FullyQualifiedName~Fellowship|FullyQualifiedName~Allegiance|FullyQualifiedName~GenerationReset"43 passed, 0 failed, 0 skipped on the committed Release binaries. The ledger's 13,201/4/0 full-suite claim was not independently re-measured (no build permitted for this review).


4. Disposition summary

# Severity Finding
MF-1 MUST-FIX Allegiance survives generation reset; retail clears at OnEndCharacterSession, the cited precedent clears, the graphical host can enter a different character in the same process, and the behavior is test-pinned
MF-2 MUST-FIX 0x027C seeds the owner; retail's handler is print-only over a stack temporary. Carries a stale-Rank second-order defect
SF-1 SHOULD-FIX Unconditional delegate holes remove 9 event types from the unhandled-count diagnostic
SF-2 SHOULD-FIX Disposed check outside the lock in both owners (ownership-ledger convergence risk)
SF-3 SHOULD-FIX RecalculateEvenXPSplitting not modelled on upsert/removal
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 = 13Complete = 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 <see cref>s) — no consumer depends on an ordinal and nothing serializes it, so the +1 shift is inert. The test is inverted, not deleted: FellowshipClearsAtResetButAllegianceSurvivesReconnectFellowshipAndAllegianceBothClearAtGenerationReset, 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 privateinternal with <InternalsVisibleTo Include="AcDream.Runtime" /> 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 @0x00569FA0AllegianceProfile::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.