Both MUST-FIX findings are on RuntimeAllegianceState and they compound. MF-1: the owner participates in no reset stage. Retail does the opposite at the same boundary -- ClientAllegianceSystem:: OnEndCharacterSession @0x00569FA0 tail-calls AllegianceProfile::Clear, while its sibling ClientFellowshipSystem::OnEndCharacterSession @0x005690A0 deletes m_pFellowship (so FA2's fellowship half IS faithful). The precedent the code and lane D §1.3 both cite, RuntimeCharacterOptionsState.HasServerSeed, CLEARS at ResetSession and its own doc names this hazard. The graphical host passes no character selector, so TrySelectFirstAvailable re-resolves the character from a fresh server list every generation -- a cross-character reset is not precluded, and nothing in the owner keys on identity. Already pinned by a passing test. MF-2: ApplyInfoResponseSelf seeds from 0x027C. Retail's dispatcher @0x006A7470 unpacks into a stack-local profile and its handler @0x0056A1D0 only prints; 0x0020's handler @0x0056A120 is the single inbound writer of the cached profile. Carries a stale-Rank second-order defect (0x027C has no rank field). Verified clean and re-derived from the decomp: all six fellowship lifecycle rules, the exact leader hand-off condition (case 8 vs case 0xC at @0x0049034B/@0x004903EF), the dispatcher-folding correction and byte-identical @allegiance info output, D4's 0x00A6 present but never fired, both bindings sites symmetric, IRuntimeEventObserver untouched, TS-81 honest, and the 8-edit J-owner template incl. teardown masks. 43/43 targeted Runtime tests pass on the committed Release binaries. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
478 lines
26 KiB
Markdown
478 lines
26 KiB
Markdown
# 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:
|
||
|
||
```csharp
|
||
// 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**:
|
||
|
||
```csharp
|
||
// 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 struct`s 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` |
|