docs: FA2 mechanism review -- APPROVE-WITH-FIXES (2 MUST-FIX, 6 SHOULD-FIX)
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>
This commit is contained in:
Erik
parent
2e97924801
commit
63649c8053
1 changed files with 478 additions and 0 deletions
478
docs/research/2026-08-12-fa2-review-mechanism.md
Normal file
478
docs/research/2026-08-12-fa2-review-mechanism.md
Normal file
|
|
@ -0,0 +1,478 @@
|
|||
# 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` |
|
||||
Loading…
Add table
Add a link
Reference in a new issue