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>
This commit is contained in:
Erik
parent
c4b7247484
commit
bc693728a6
1 changed files with 500 additions and 0 deletions
500
docs/research/2026-08-12-fa1-review-mechanism.md
Normal file
500
docs/research/2026-08-12-fa1-review-mechanism.md
Normal file
|
|
@ -0,0 +1,500 @@
|
|||
# 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` | ✔ |
|
||||
| 4–7 | 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.
|
||||
Loading…
Add table
Add a link
Reference in a new issue