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>
39 KiB
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 FA3–FA5
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 = 7 → RuntimeCharacterState.ResetSession →
Try(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 FA3–FA5 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 @0x00490D59 →
Event_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::AddTextToScroll — it 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 @0x005B9730 → Fellow::operator= on hit, AddFellow on miss (:457071-457098) |
| SELF quit clears the WHOLE snapshot | ApplyQuit, quitterGuid == selfGuid → ClearLocked() (:110-124) |
Handle_Fellowship__Quit @0x00569E90: arg2 != player_id → RemoveFellow, 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 @0x00569E70 → DeleteFellowship (: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_Inner → FellowshipQuit(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.TryGetValue → registration.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.SendFellowshipUpdateRequest →
SocialActions.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 = 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 <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:
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 <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 nowComplete & ~Fellowship & ~Allegiance & ~Identity & ~EntityObjects= 9 flags. I re-derived the whole table: at_disposeStage == Nexactly N flags are set, for every N in 0..13. The newCompletedTeardownStagesAccumulatesExactlyOneFlagPerStagereflection walk pins every intermediate stage (the previous tests only sampled stage 0 andComplete, which is precisely why the off-by-one survived), and its closingAssert.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_RoutesSelfGuidToClearAndOtherGuidToRemovebuilds realRuntimeFellowshipState/RuntimeAllegianceStateowners, wires them through the one productionLiveSessionEventRouterregistration site withPlayerGuid: () => self, and dispatches real0x00A3envelopes 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. ResetSessionguard alignment (blast SF-5) — FIXED, see MF-1 above.GetVassalsallocation (blast SF-7) — documented as an intentional exception with the reason (yield returnis illegal inside alock) 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
RuntimeCharacterOptionsStatecitation, §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 the0x01C9/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 —SendAllegianceInfoRequestpre-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 landed0x0020 AllegianceUpdateis the sole seeder, matching retail's soleAllegianceProfile::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.