acdream/docs/research/2026-08-12-fa1-review-mechanism.md
Erik bc693728a6 docs: FA1 mechanism review — APPROVE-WITH-FIXES (2 MUST-FIX, 5 SHOULD-FIX)
Mechanism-faithfulness lens on 7be86f47/6bedbc47/5f9aa16f/4281750b.

Every wire layout re-derived independently from the retail decomp rather
than taken on the lane docs' word: Fellow::UnPack, Fellowship::UnPack,
PackableHashTable::UnPack's count/buckets split, DispatchUI_UpdateFellow's
guid-first read, Event_Create's single trailing u32 shareXP,
AllegianceHierarchy::UnPack's eleven gates + their non-monotonic wire
order, AllegianceHierarchy::Add, AllegianceProfile::UnPack, the 0x20
dispatch case. All six golden byte vectors re-computed field by field --
no encoding, padding, or endianness slip found.

Both premise-contradiction calls VERIFIED CORRECT from primary source:
the 8 fellowship WeenieError ids genuinely have no case label, no else-if
comparison, no decimal form and no default fallthrough in
HandleFailureEvent (D9's premise was wrong, the refusal to invent English
was right); and 0x0275 is client-authored, so the typed ConfirmationType
enum -- not a receive parser -- was the real gap, and D6's FA4/FA5 flows
are buildable on what landed.

MUST-FIX: (1) AllegianceHierarchy::Add's fourth rejection rule (_id == 0,
which also makes treeParent == 0 unconditionally fatal) is unmodeled;
(2) the 0x001F AllegianceUpdateRequest builder -- the allegiance twin of
the 0x00A6 this slice repaired, and lane C's #3 minimum-viable message --
is missing entirely.

SHOULD-FIX: monarch MayPassupExperience is not force-cleared;
ParseFellowshipDisband validates a body length retail never inspects;
D5's <<1 shape is unpinned at the 0x02C0 site; AP-90's register row still
cites the deleted AllegianceTree; the gate comment numbering stops at ten.

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

500 lines
25 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

# Campaign FA slice FA1 — MECHANISM-FAITHFULNESS review
**Date:** 2026-08-12. **Lens:** mechanism faithfulness (is every byte and
every rule right?). **Mode:** read-only; this doc is the only file written.
**Target:** `7be86f47` (builders), `6bedbc47` (parsers), `5f9aa16f`
(WeenieError), `4281750b` (delete `AllegianceTree`), ledger `ee1124ca`.
**Contract:** `docs/plans/2026-08-11-fellowship-allegiance-campaign.md`
slice FA1 + D3/D5/D6/D9, with
`docs/research/2026-08-11-fa-fellowship-wire.md` (lane B) and
`docs/research/2026-08-11-fa-allegiance-wire.md` (lane C) as ground truth.
**Primary sources re-read independently for this review** (not taken on
the lane docs' word): `docs/research/named-retail/acclient_2013_pseudo_c.txt`
(`Fellow::UnPack @0x005B9AD0`, `Fellowship::UnPack @0x005B94F0`,
`PackableHashTable<unsigned long,Fellow>::UnPack @0x00569A90`,
`CM_Fellowship::DispatchUI_UpdateFellow @0x006A6700`,
`CM_Fellowship::Event_Create @0x006A67A0`,
`AllegianceHierarchy::UnPack @0x005B7520`,
`AllegianceHierarchy::Add @0x005B6E90`,
`AllegianceProfile::UnPack @0x005B6B90`, the `0x20` dispatch case at
`0055b5dd`, `ClientCommunicationSystem::HandleFailureEvent @0x00571990`),
`docs/research/named-retail/acclient.h:2979-2994` (`AllegianceVersion`),
`docs/research/named-retail/symbols.json`, and the ACE writers
`references/ACE/Source/ACE.Server/Network/Structure/AllegianceHierarchy.cs`,
`.../AllegianceData.cs`, `.../AllegianceProfile.cs`,
`.../GameEvent/Events/GameEventAllegianceUpdate.cs`.
---
## VERDICT: **APPROVE-WITH-FIXES**
Every wire layout in this slice is byte-correct against the retail
decompilation, and both premise-contradiction calls are **independently
verified correct from primary source** (§2). The fixes below are two
rule/coverage gaps and a set of smaller fidelity items; none of them
invalidates a landed byte vector, and all of them are unreachable against
ACE today — which is exactly why they need pinning now rather than
discovering them against a non-ACE server or a hand-built FA2 fixture.
**MUST-FIX: 2. SHOULD-FIX: 5. Nits (non-blocking): 5.**
---
## 1. MUST-FIX
### MF-1 — `AllegianceHierarchy::Add`'s FOURTH rejection rule (`_id == 0`) is not modeled
`src/AcDream.Core.Net/Messages/ClientCommandResponses.cs:501-524` models
three of the four rejection rules in
`AllegianceHierarchy::Add @0x005B6E90`. The whole function body in retail
is wrapped in `if (_id != 0)`:
```
005b6e96 uint32_t _id = arg3->_id;
005b6e9e if (_id != 0) <-- every success path is inside this
...
005b6ecd return 0; <-- _id == 0 falls straight out
```
and the caller (`005b779d`) turns any `Add` failure into `return 0` from
`UnPack`, i.e. the whole message is discarded. Lane C §4.4's own
pseudo-code lists it first (`if (data._id == 0) return 0; // reject`).
acdream reads the monarch at `:505` and each record at `:512` with no
`CharacterId != 0` check, so:
* a profile whose **monarch** record has id 0 is accepted where retail
discards it, and
* every subsequent record naming `treeParent = 0` is then admitted by
`knownIds.Contains(0)` — where retail rejects `treeParent == 0`
unconditionally (`005b6ee5`: `esi = (arg2 != 0) ? Search(...) : nullptr`,
then `esi != 0` is required).
One guard fixes both: reject the whole message when any record's
`CharacterId == 0`. Add the boundary test beside the existing three at
`tests/AcDream.Core.Net.Tests/Messages/AllegianceProfileVersionGateTests.cs:312-353`.
Unreachable against ACE (`AllegianceData.cs:69` always writes a real
guid) — the same "correct only because ACE is generous" class lane C §0
warns about for the version gates, which this slice otherwise closed.
### MF-2 — `0x001F AllegianceUpdateRequest` builder is missing
`src/AcDream.Core.Net/Messages/AllegianceRequests.cs:34-83` ships
Swear (`0x001D`), Break (`0x001E`) and the new Kick alias — and nothing
else. There is no `0x001F` builder anywhere in the tree (repo-wide grep
for `0x001F` / `AllegianceUpdateRequest` over `src/**/*.cs` returns only
`PropertyBool.AllegianceUpdateRequest`, an unrelated property id).
`0x001F` is the allegiance panel's subscribe/unsubscribe toggle — the
exact structural twin of the fellowship `0x00A6` this slice *did* repair
and rename. Lane C §1.2 pins all four retail send sites
(`gmAllegianceUI::PostInit` tail `@0x004911C6` → 1,
`RecvNotice_PlayerDescReceived @0x00490D59` → 1,
`OnVisibilityChanged` visible `@0x004912DD` → 1, hidden `@0x00491311` → 0)
and lane C §7.1 numbers it #3 of the six messages the minimum-viable
panel needs, with the explicit instruction "ACE ignores the value but
retail servers do not; send it correctly."
FA1 is the Core.Net truth slice and FA2 (Runtime owners + typed commands)
lands before FA5; leaving this out pushes a `AcDream.Core.Net` builder
addition into a Runtime or panel slice. Six lines plus a golden vector
(`[u32 0xF7B1][u32 seq][u32 0x1F][u32 on]`, total `0x10` per lane C §3.2).
---
## 2. The two premise-contradiction calls — BOTH VERIFIED CORRECT
I re-derived both from primary source rather than accepting the commit
messages. Both stand.
### (a) The 8 fellowship `WeenieError` ids genuinely have no retail text — `5f9aa16f` is right, D9's premise was wrong
D9 asserted "the 8 missing fellowship WeenieError strings are added in
FA1 … all resolve from the DAT string tables, never invented." The
implementer refused and documented absence instead. Verified:
1. **Case-label walk.** In `HandleFailureEvent @0x00571990`
(`acclient_2013_pseudo_c.txt:382616`+) the label sequence is
`case 0x413: → case 0x414: → case 0x416: → case 0x41d: → case 0x41e:
→ case 0x41f:`**no label for `0x417``0x41C`** — and
`case 0x4d9: → case 0x4da: → case 0x4dd:` — **no label for `0x4DB`/
`0x4DC`**.
2. **The `else if` chain too.** `WeenieErrorMessages`' own class doc warns
that five ids dispatch via `else if (arg2 == N)` above the switch, so
a case-label-only sweep would be unsound. The function contains exactly
seven such comparisons — `0x43`, `0x3f7`, `0x3e`, `0x23`, `0x36`,
`0x4e8`, `0x48a`, `0x408`, `0x3ee`, `0x4f` — none of them ours.
3. **Whole-file sweep.** `\b0x417\b``\b0x41c\b`, `\b0x4db\b`,
`\b0x4dc\b` across all 1.4 M lines return only unrelated hits: struct
field offsets in three `*(uint16_t*)(ecx+0x418)` copy loops, and
`[0x417]`/`[0x418]`/… array-index headers inside a `MeshBuffer`
render-batch data dump at `.data 0x008872xx` / `0x008c4axx`. Nothing
textual, nothing comparative.
4. **Decimal forms.** No `1047``1052`/`1243`/`1244` anywhere in the
function's address range.
5. **No `default:`** in the switch, so an unmatched id produces no text at
all — which is precisely what `Resolve()` already returns.
Conclusion: acdream's existing silence for `0x0417 FellowshipIgnoringRequests`
and `0x04DB FellowshipDeclined` (the two ACE actually sends) is
retail-faithful. Adding invented English would have been the exact
failure mode the table's no-default-case rule exists to prevent. The
conformance `[Theory]` at
`tests/AcDream.Core.Tests/Chat/WeenieErrorMessagesTests.cs:390-393` is the
right artifact. **No fix owed; no register row owed** (silence *is* the
retail behavior, so there is no deviation to record).
### (b) `0x0275` is client-authored — a receive parser was never the ask, and D6's need is satisfied
Verified: `0x0275` is a C→S GameAction sub-opcode
(`CM_Character::Event_ConfirmationResponse @0x006A1210`, buffer `0x18`;
consumed server-side by ACE's `GameActionConfirmationResponse`). Retail
never receives it. So "the missing third message of the triple" (D6)
could not have meant a parser.
`ClientCommandRequests.BuildConfirmationResponse` already existed pre-FA1
byte-for-byte (`git show 6bedbc47~1` — 24-byte body,
`[env][seq][0x0275][type][context][accepted]`, matching retail's `0x18`
allocation exactly). What was genuinely absent was the **typed
discriminator**, and `GameEvents.ConfirmationType`
(`src/AcDream.Core.Net/Messages/GameEvents.cs:552-561`) supplies it with
all seven values matching retail's
`ClientUISystem::Handle_Character__ConfirmationRequest @0x005640A0` switch
(symbol verified at that exact address) and ACE's enum.
**D6's FA4/FA5 flows are buildable on this today**: receive `0x0274` via
`ParseCharacterConfirmationRequest` (`:512`, `Type` as `uint` — cast to
the new enum), send via `BuildConfirmationResponse((uint)ConfirmationType.Fellowship, …)`,
receive `0x0276` via `ParseCharacterConfirmationDone` (`:533`). The
reinterpretation is sound.
---
## 3. SHOULD-FIX
### SF-1 — the monarch's `MayPassupExperience` must be forced false
`AllegianceHierarchy::UnPack`'s last act before returning success is
`AllegianceData::SetMayPassupExperience(&m_pMonarch->_data, 0)`
(`acclient_2013_pseudo_c.txt:454670`, `005b77b3`) — the monarch can never
pass up, whatever the wire bit says. Lane C §4.1 names this in the same
paragraph as the `HasPackedLevel`-absent rule that
`ClientCommandResponses.cs:564-565` *did* port; only one of the pair
landed. The field is public
(`AllegianceMemberRecord.MayPassupExperience`, `:201`), so FA5 could
render it wrong. Clear it on the monarch record at `:505-506`.
Inert against ACE (`AllegianceData.cs:76` never sets the bit for a
monarch), but it becomes live the moment the record is hand-built or a
non-ACE server is used.
### SF-2 — `ParseFellowshipDisband` rejects a non-empty body; retail never does
`src/AcDream.Core.Net/Messages/GameEvents.cs:749`:
`ParseFellowshipDisband(payload) => payload.Length == 0`. Retail's
`CM_Fellowship::DispatchUI_Disband @0x006A5E80` reads **only** the opcode
and then calls the handler — it never inspects or validates a trailing
byte. Encoding a length check retail does not perform means any future
non-empty body (an ACE change, a trailing pad) silently swallows the
disband and leaves FA2's roster stuck in a fellowship the server already
destroyed. The faithful contract is unconditional acceptance. The
matching test at
`tests/AcDream.Core.Net.Tests/Messages/FellowshipEventsTests.cs:197-201`
pins the unfaithful half and should flip with it.
### SF-3 — D5's `<<1` shape is not pinned at the `0x02C0` site
D5 requires the raw-uint `shareLoot` read at **both** message sites. The
mechanism is right — `ReadFellow` (`GameEvents.cs:777`, `shareLoot` at
`:786`) is shared by `ParseFellowshipFullUpdate` (`:661`) and
`ParseFellowshipUpdateFellow` (`:706`), and `FellowMember.ShareLoot`
(`:622`) is a `uint` with no `== 1` comparison anywhere in the tree. But
the tests pin `0x10` and `2` only at `0x02BE`
(`FellowshipEventsTests.cs:70,95`); the `0x02C0` fixture writes
`shareLoot = 0` (`:154`). Add the `<<1`-shape (`2`) assertion at the
`0x02C0` site so a future refactor that splits `ReadFellow` cannot
silently reintroduce a bool there.
Carry-forward for the FA2 reviewer: `!= 0` semantics are currently only
*documented* (`:606-609`) — nothing consumes `ShareLoot` yet. The
consuming comparison lands in FA2 and must be checked there.
### SF-4 — AP-90's register row now cites a deleted class
`docs/architecture/retail-divergence-register.md:298` (AP-90) reads
"… and its `AllegianceTree` is not wired into GameWindow". `4281750b`
deleted that class. The row itself is still valid (radar relationship
state is still not delivered), but the prose must be re-pointed at
`ClientCommandResponses.AllegianceProfileLookups` / the FA2 owner. The
register rule says the bookkeeping rides the same commit; this one
slipped.
### SF-5 — the "eleven gates" numbering in the code comments tops out at ten, and one lane citation does not exist
Two documentation-accuracy items that will cost a future auditor time:
* `ClientCommandResponses.cs:411-491` labels the gates "1/2, 3, 4, 5, 6,
7, 8, 9, 10". The contract, the commit message and lane C all say
**eleven**. All eleven *fields* are gated correctly (verified §4 below)
— only the comment numbering is off, and it is the exact artifact
someone would use to answer "are all eleven present?".
* `SocialActions.cs:184` cites "lane B §4.5, §12". Lane B has no §12 —
the intended reference is master-table **row 12**
(`Panel visibility → update request`).
---
## 4. Verified clean (re-derived, not taken on trust)
### 4.1 Golden byte vectors — all six independently re-derived, all correct
| Builder | Expected bytes (hand-derived from the cited retail sender) | Test | Verdict |
|---|---|---|---|
| `BuildFellowshipCreate(seq 7,"Team",true)` | `B1F70000 07000000 A2000000` + str16L`0400 54 65 61 6D 0000` + `01000000` = 24 B | `SocialActionsTests.cs:65-83` | ✔ |
| `BuildFellowshipCreate(seq 1,"X",false)` | 12 + 4 (str16L `0100 58 00`) + 4 = 20 B, tail `00000000` | `:85-95` | ✔ |
| `BuildFellowshipUpdateRequest(9,true)` | `…A6000000 01000000` | `:130-146` | ✔ |
| `BuildFellowshipAssignNewLeader(2,0x50001234)` | `…90020000 34120050` | `:151-168` | ✔ |
| `BuildFellowshipChangeOpenness(4,false)` | `…91020000 00000000` | `:173-190` | ✔ |
| `BuildSwear/BuildBreak/BuildKick` | `…1D000000 AAAA0000` / `…1E000000 BBBB0000` / `…1E000000 42000050` | `AllegianceRequestsTests.cs:39-90` | ✔ |
The `0x00A2` repair is confirmed at the strongest possible provenance:
`CM_Fellowship::Event_Create @0x006A67A0` allocates `strPackSize + 0x10`,
stores `0xa2` at `006a6809`, packs the string at `006a6828`, then
`*(uint32_t*)var_c = arg2` at `006a6835` — **one trailing u32 shareXP and
no openness field**. The prior two-byte `openness`/`shareXp` shape (and
the test that pinned it) was exactly the inversion lane B §5.2(a)
described.
`PackString16L` (`SocialActions.cs:389-403`) is Windows-1252 with the pad
computed over `2 + length` — matching retail's `PStringBase::Pack`, ACE's
`WriteString16L`, and the reader `ReadString16L`
(`GameEvents.cs:913-926`). No endianness, padding, or encoding slip
anywhere in the set.
Byte-write-vs-u32-write equivalence (`body[12] = 0|1` into a zeroed
16-byte buffer) is byte-identical to retail's full-word store for these
0/1 payloads — see nit N-2.
### 4.2 `Fellow` record layout (the load-bearing one)
`Fellow::UnPack @0x005B9AD0` reads, in order: `_cp_cache`, `_lum_cache`,
`_level`, `_max_health`, `_max_stamina`, `_max_mana`, `_current_health`,
`_current_stamina`, `_current_mana`, `_share_loot`, then `str16L _name`
(min size `0x28`). `GameEvents.ReadFellow` (`:777-793`) is field-for-field
identical. ✔
### 4.3 `0x02BE` field order and the hash-table header
`Fellowship::UnPack @0x005B94F0`: `_fellowship_table``_name`
`_leader``_share_xp``_even_xp_split``_open_fellow``_locked`
`_fellows_departed`, then returns — it never reads field 9 (locks).
`ParseFellowshipFullUpdate` (`GameEvents.cs:637-681`) matches exactly,
**including** the deliberate non-parse of field 9, which is the
retail-faithful choice (lane B §3.9 suggested reading it; retail does
not, and lane B's own U3 flags the struct width as unverified — the
implementer's stop-and-document call is correct and needs no register
row).
Header: `PackableHashTable<unsigned long,Fellow>::UnPack @0x00569A90`
reads one u32 and splits `_table_size = v >> 16`, `count = v & 0xFFFF`
so the wire is `[u16 count][u16 numBuckets]` LE, which is what the parser
reads for both the member table (`:653-654`) and the departed table
(`:670-671`). ✔
### 4.4 `0x02C0` is guid-first — the Chorizite disagreement independently re-resolved
`CM_Fellowship::DispatchUI_UpdateFellow @0x006A6700`:
```
006a6726 cond = *(uint32_t*)eax_3 == 0x2c0; // opcode at +0
006a673b uint32_t edi = *(uint32_t*)(eax_3 + 4); // MEMBER GUID at +4
006a6763 Fellow::UnPack(&var_30, &arg2, …); // Fellow from +8
006a676c uint32_t ecx_4 = *(uint32_t*)eax_7; // updateType last
```
Retail puts the guid first. `ParseFellowshipUpdateFellow`
(`GameEvents.cs:700-712`) reads guid → `Fellow``updateType`. ✔ The
lane-B three-source resolution (retail + ACE + holtburger over
Chorizite's generated shape) is confirmed at its retail end.
### 4.5 The eleven version gates — every threshold and the ORDER
Re-derived from `AllegianceHierarchy::UnPack @0x005B7520` and the
`AllegianceVersion` enum (`acclient.h:2979-2994`). Order on the wire is
**not** monotonic in version — officer titles (v9) precede the pools
block (v2) — and the implementation follows the wire, not the numbering:
| # | Field | Retail predicate (address) | `ClientCommandResponses.cs` | ✔ |
|---|---|---|---|---|
| 1 | officers `PHashTable<u32,u32>` | `m_oldVersion >= 6` (`005b7570`/`005b7585`) | `:416-425` | ✔ |
| 2 | legacy spokesperson 4-byte skip | `1 <= v < 6` (`005b759b`) | `:426-429` | ✔ |
| 3 | officer titles `PSmartArray<PString>` (bare i32 count) | `>= OfficersTitlesAdded(9)` (`005b75b4`) | `:434-439` | ✔ |
| 47 | monarch/spokes broadcast time+today ×4 | `>= PoolsAdded(2)` (`005b75e0`) | `:442-448` | ✔ |
| 8 | `m_motd`, `m_motdSetBy` | `>= MotdAdded(3)` (`005b7645`) | `:453-457` | ✔ |
| 9 | `m_chatRoomID` | `>= ChatRoomIDAdded(4)` (`005b7696`) | `:461-462` | ✔ |
| 10 | `m_BindPoint` (32-byte Position) | `>= Bindstones(7)` (`005b76b2`) | `:468-472` | ✔ |
| 11 | `m_AllegianceName` + `m_NameLastSetTime` | `>= AllegianceName(8)` (`005b76d1`) | `:477-481` | ✔ |
| 12 | `m_isLocked` | `>= LockedState(10)` (`005b770d`) | `:485-486` | ✔ |
| 13 | `m_ApprovedVassal` | `>= ApprovedVassal(11)` (`005b7728`) | `:490-491` | ✔ |
`BannedCharactersAdded(5)` gates nothing — confirmed, and pinned by the
negative test `VersionGate_4to5_BannedCharactersAddedGatesNothing`.
**Boundary tests sit on the boundaries.** Every gate has an `N-1`/`N`
fixture pair: 0→1, 1→2, 2→3, 3→4, 4→5 (negative), 5→6, 6→7, 7→8, 8→9,
9→10, 10→11
(`AllegianceProfileVersionGateTests.cs:152-308`). For the gates whose
fields are not surfaced (pools, bind point, officers, titles) the
assertion is the monarch record surviving intact — which IS a real
structural proof here, because the fixture writer
(`BuildProfileWire`, `:40-134`) applies the gate conditions
independently of the parser: a wrong threshold would consume or skip
bytes the writer did not emit and garble every field after it.
Cross-checked against ACE's writer
(`AllegianceHierarchy.cs:140-155`): identical field order, and
`oldVersion = 0x000B` always (`:50`) — so these gates are inert against
ACE and the live `@allegiance info` path is unchanged, exactly as
intended.
### 4.6 `AllegianceData` field order and `AllegianceProfile`/`0x0020` framing
`AllegianceData` per lane C §4.1, cross-checked against ACE
`AllegianceData.cs:95-117`: id, cpCached, cpTithed, bitfield, u8 gender,
u8 heritage, u16 rank, [u32 level if `HasPackedLevel`], u16 loyalty,
u16 leadership, [8 bytes either branch], str16L name.
`ReadAllegianceData` (`:532-571`) matches. ✔
`AllegianceProfile::UnPack @0x005B6B90` = `totalMembers`, `totalVassals`,
hierarchy — matches `:406-409`. ✔
The `0x20` dispatch case (`0055b5dd`-`0055b607`) reads one u32 and then
`AllegianceProfile::UnPack`, and ACE's `GameEventAllegianceUpdate.cs:22-25`
writes `rank` then the profile — so `ParseAllegianceUpdate`'s
rank-leading + shared-body split (`:375-391`) is right, as is the
guid-leading `0x027C` sibling. Sharing one `ReadAllegianceProfileBody` is
exactly lane C §7.2's reuse verdict honored. ✔
### 4.7 Tree rules — three of four correct, and the reversal test pins ORDER
Orphan / self-parent / duplicate all match `Add`'s semantics:
* orphan — retail `esi = (arg2 != 0) ? Search(arg2, monarch) : nullptr;
if (esi == 0) return 0` ≡ `!knownIds.Contains(parentGuid)` (`:514`);
* self-parent — retail `esi->_data._id != _id` required, and `esi` was
found *by* `treeParentId`, so it is `treeParentId == _id` ≡
`parentGuid == record.CharacterId` (`:515`);
* duplicate — retail `Search(_id, monarch) == 0` required ≡
`knownIds.Contains(record.CharacterId)` (`:516`);
* all three `return null`, i.e. whole-message discard, matching
`UnPack`'s `return 0` at `005b779d`. ✔
The record loop's first-record-has-no-`treeParent` shape is confirmed at
`005b776e` (`if (ebp_1 != 0) { read treeParent }`) and matches `:505-512`.
✔ (The fourth rule is MF-1.)
**Sibling reversal — the test pins the ORDER, not just membership.**
`FindVassals` (`:288-294`) walks the record list backwards, which is the
correct model of retail's push-front insert (`node->_peer = parent->_vassal;
parent->_vassal = node;` at `005b6f15`-`005b6f38`) followed by a
`GetFirstVassal`/`GetNextVassal` walk.
`TreeAssembly_SiblingOrder_ReversesOnAssembly`
(`AllegianceProfileVersionGateTests.cs:371-396`) asserts the exact
sequence `{"VassalC","VassalB","VassalA"}` against wire order A,B,C. ✔
Note this changes the live `@allegiance info` vassal render order (it was
forward before FA1) — the new order is the retail one; lane C §9 U3 flags
a side-by-side eyeball as the closing check.
### 4.8 Dead opcodes `0x01C9`/`0x01CA`
`ParseFellowshipFellowUpdateDone` / `…StatsDone`
(`GameEvents.cs:759-775`) return a non-nullable record and cannot fail:
empty payload → `RawValue = null`, trailing u32 → captured. Both cases
are tested (`FellowshipEventsTests.cs:205-233`). Nothing user-visible is
possible — neither is registered in `GameEventWiring.WireAll` (grep for
`Fellowship` there returns nothing), and `RawValue` has no consumer.
Matches lane B §2.7's parse-and-ignore instruction and its BN-zero-fold
caution (payload tolerated, not assumed empty). ✔
### 4.9 Retail anchors in code comments — spot-check (10, not 5)
Resolved against `symbols.json` (exact-address match unless noted):
| Anchor cited in code | Result |
|---|---|
| `gmFellowshipUI::CreateFellowship @0x0048F730` | exact ✔ |
| `gmFellowshipUI::OnVisibilityChanged @0x0048E460` | exact ✔ |
| `Event_AssignNewLeader @0x006A5F70` | exact (`CM_Fellowship::`) ✔ |
| `Event_ChangeFellowOpeness @0x006A6040` | exact (`CM_Fellowship::`) ✔ |
| `AssignLeadershipToFellow @0x0048F1E0` | exact ✔ |
| `Fellow::Pack @0x005B9A10` / `UnPack @0x005B9AD0` | exact ✔ |
| `AllegianceHierarchy::Add @0x005B6E90` | exact ✔ |
| `Handle_Character__ConfirmationRequest @0x005640A0` | exact (`ClientUISystem::`) ✔ |
| `gmAllegianceUI::CloseKickConfirmationDialog @0x00490B00` | exact ✔ |
| `gmFellowshipUI` Quit button case 8 `@0x0049034B`; `Event_BreakAllegiance @0x006A69DA` | instruction-level, correctly inside `gmFellowshipUI::ListenToElementMessage @0x004901C0` and `CM_Allegiance::Event_BreakAllegiance @0x006A6980` ✔ |
No misattributed anchor found.
### 4.10 Other contract items
* All **11 unwired S→C parsers** are present (fellowship `0x00A3`,
`0x00A4`, `0x01C9`, `0x01CA`, `0x02BE`, `0x02BF`, `0x02C0`; allegiance
`0x0003`, `0x0020`, `0x01C8`, `0x027A`), and all 11 ids exist in
`GameEventType`. ✔
* `AllegianceTree` deletion is clean — no production or test reference
survives (the one `AllegianceTree` hit in `ChatChannelInfoTests.cs:28`
is an unrelated test-method name). Lane C §7.3's DELETE verdict
honored. ✔ (See SF-4 for the register prose.)
* Test fixtures are generated by `AceWireWriter`, a line-for-line mirror
of ACE's `Extensions.cs` writer — provenance is the server's own
algorithm, not hand-typed hex. ✔
* No stale callers of the renamed/re-signatured builders remain. ✔
---
## 5. Nits (non-blocking; listed for completeness, no action required)
* **N-1** — Retail degrades *gracefully* on a short legacy blob (each
gated read is additionally guarded by "≥4 bytes remain", e.g.
`005b759b`, `005b75fc`); acdream throws `FormatException` and discards
the message instead. Only reachable on a truncated pre-v11 blob, which
ACE cannot produce.
* **N-2** — `BuildFellowshipQuit`/`BuildFellowshipUpdateRequest`/
`BuildFellowshipChangeOpenness` write a single byte where retail writes
a full u32. Byte-identical for the 0/1 payloads these carry (zeroed
buffer), and blessed by lane B §5.2 — noted only so nobody "fixes" it
into a behavior change.
* **N-3** — Retail's record count is a **signed** `int16_t`
(`005b7546`: `int16_t i = ecx;` … `if (i > 0)`), so a header with the
high bit set reads as zero records in retail and as up to 32 767
records in acdream. Bounded and unreachable against ACE.
* **N-4** — `GameEvents.ParseConfirmationResponse` (`:581`) is by
construction unreachable in production (nothing receives `0x0275`); its
only consumer is `ConfirmationTripleTests`. Justified in its own doc
comment as a round-trip conformance mechanism, and it is the cheapest
way to prove the builder against a second implementation — but it is
new never-called code in a slice whose D3 rule is "connected, repaired,
or deleted — never trusted", so FA2 should either route it or leave the
doc comment intact as the standing justification.
* **N-5** — `4281750b`'s commit message repeats lane C §0's "~1000×"
figure for the deleted `AllegianceMath.ComputePassup`. The actual error
is roughly 40× at maxed loyalty and *under*-states at low loyalty
(`(50 + 22.5·291)/291 = 22.67` vs a true passup factor of ≈`0.53`).
The DELETE verdict is unaffected and correct; the magnitude claim
originates in the lane doc, not the implementer.
---
## 6. Note on evidence I could not produce
The slice's automated gate (Release build + full suite 13,149/4/0) is the
regression reviewer's lane. I did not build. The Debug test binaries in
this worktree predate FA1 (a `--no-build --filter` run over the six FA1
test classes matched only the 16 pre-existing `SocialActionsTests`/
`AllegianceRequestsTests` cases), so per the stale-build-artifact rule I
treat that run as evidence of nothing and reviewed every new test by
reading it instead. All test *logic* above is verified by inspection
against the primary sources cited.