acdream/docs/research/2026-08-12-fa1-review-blast.md

Campaign FA slice FA1 — BLAST-RADIUS review

Date: 2026-08-12 Reviewer lens: blast radius — what else could these changes have touched, along every axis the implementer may not have traversed (the C5b lesson: enumerations leak along the axis not searched). Target: 7be86f47, 6bedbc47, 5f9aa16f, 4281750b, ee1124ca on claude/latest-commits-cb0c8f (diff base bb48d2c8). Method: read-only. Repo-wide greps (src, tests, tools, docs, .claude, in-repo memory/, persistent ~/.claude/.../memory/, AcDream.slnx), retail decomp cross-check against docs/research/named-retail/acclient_2013_pseudo_c.txt, ACE writer/handler cross-check, and targeted dotnet test --no-build -c Release on the current (00:13 Aug 12, contemporaneous with the commits) binaries. No build, no client launch, no subagents.

---

VERDICT: APPROVE-WITH-FIXES

The wire work is right. I independently re-derived the two repaired builders against ACE's own handlers, all eleven allegiance version gates against retail's AllegianceHierarchy::UnPack, and both tree-assembly rules against AllegianceHierarchy::Add. Every one checks out, including the two claims that most deserved scepticism (the sibling-order reversal and the whole-message discard). Nothing in FA1 double-registers, shadows an existing handler lane, or touches a cross-host path.

The bookkeeping is not right, and one of the gaps is the exact failure mode the C4 closeout handoff names as the most expensive in that campaign — a written contract that asserts a mechanism which no longer exists, left in place for the next slice to execute. FA1 falsified its own successor's seam map in four places and corrected none of them.

Four MUST-FIX (all documentation/bookkeeping, no code), five SHOULD-FIX.

---

MUST-FIX

MF-1 — The divergence register still cites the class FA1 deleted

docs/architecture/retail-divergence-register.md:298 (row AP-90):

"…acdream does not yet maintain live fellowship membership and its AllegianceTree is not wired into GameWindow."

4281750b deleted src/AcDream.Core/Allegiance/AllegianceTree.cs. CLAUDE.md's register rule is binding in both directions — a commit that retires the cited mechanism updates the row in the same commit. The deviation AP-90 records is still real (radar relationship blips are modeled but undelivered), so this is an edit, not a retirement: the row's evidence should now point at the flat AllegianceMemberRecord list + AllegianceProfileLookups that replaced the tree class, and at the fellowship parsers FA1 added.

This is also the row a future reader would hit first when asking "why doesn't the radar show fellows?" — it currently sends them to a file that does not exist.

MF-2 — FA2's and FA6's written contract was falsified and left in place

docs/research/2026-08-11-fa-acdream-seams.md (lane D) is not an inert dated snapshot: the campaign plan's §1 table calls the lane docs "the spec's data appendix", and 6bedbc47's own commit message points FA2 at this file by section number. Four of its statements are now false, and one of them is actively dangerous:

Line	Says	Reality after FA1
:791	Allegiance state owner = RuntimeAllegianceState "wrapping existing AcDream.Core.Allegiance.AllegianceTree", precedent citation AllegianceTree.cs:56-162	Class deleted. FA2 is the very next slice and this is its seam-map row.
:666	commands.Fellowship.SetOpen(gen, open) → BuildFellowshipUpdate	BuildFellowshipUpdate no longer exists. Its renamed successor BuildFellowshipUpdateRequest is panel visibility, not openness. A FA6 implementer following this table literally re-introduces exactly the semantic bug FA1 just fixed. The correct target is the new BuildFellowshipChangeOpenness (0x0291).
:662, :429	BuildFellowshipCreate(seq, name, openness, shareXp) at SocialActions.cs:123-138	Now 3 parameters (seq, fellowshipName, shareXp) at :144. The openness argument the bot-command row passes has no wire field to land in.
:848	Open question 8: "BuildFellowshipCreate trailing-pad rule is pinned only by a self-consistent test, not against a retail/ACE reference. Lane A/B should confirm before FA sends it live."	Answered by FA1 — and I re-confirmed it below (VC-3). The question should be closed with its answer, not left open for someone to re-do.

Fix by amending lane D in place with dated FA1 correction notes (the campaign already treats these docs as living appendices), or by adding an explicit "FA1 contract corrections" subsection to the plan that FA2 must read first. Either is fine; leaving it is not.

MF-3 — Plan decision D9 still asserts the opposite of what shipped

docs/plans/2026-08-11-fellowship-allegiance-campaign.md:103-105:

"D9 — the 8 missing fellowship WeenieError strings are added in FA1 (two are on ACE's live send paths today; all resolve from the DAT string tables, never invented)."

and the FA1 slice-map row at :115 likewise lists "add the 8 WeenieError strings" as slice scope.

5f9aa16f shipped the opposite finding and added no strings: retail's Sept-2013 client has no display text for any of the 8, so acdream's current silence is already faithful. I agree with the finding and with refusing to invent English for them — but ee1124ca updated only the §9 ledger row, so the plan's own decision list and slice map now contradict the shipped result. The roadmap discipline is explicit that plan/reality divergence is fixed in the session it is discovered.

Note the lane B research doc itself is not wrong here (it only ever claimed the ids were absent from acdream's table, which was true) — the overstatement is the plan's alone.

MF-4 — The ledger's test totals are internally inconsistent and do not reconcile

docs/plans/2026-08-11-fellowship-allegiance-campaign.md:181:

"full suite 13,149/4 skips/0 failures (baseline 13,103/4/0, net +50 tests)"

13,149 − 13,103 = 46, not 50. The row states two totals and a delta, and no two of the three agree.

My independent count from the actual diffs:

File	Δ test cases
AllegianceProfileVersionGateTests.cs (new)	+20
AllegianceSmallEventsTests.cs (new)	+7
ConfirmationTripleTests.cs (new)	+4
FellowshipEventsTests.cs (new)	+13
SocialActionsTests.cs (14 → 17 [Fact])	+3
AllegianceRequestsTests.cs (2 → 5 [Fact])	+3
WeenieErrorMessagesTests.cs (one new [Theory], 37 → 45 InlineData)	+8
AllegianceTreeTests.cs (deleted, 9 methods, no theories)	−9
net	+49

So there are three mutually exclusive numbers (+46 implied, +50 stated, +49 measured) and at most one can be right. Most likely the baseline or the final figure was transcribed from a stale run. Re-run the full Release suite and correct the row — a ledger whose arithmetic does not close cannot serve as the automated gate it claims to be.

What I can confirm on the current Release binaries (built 00:13 Aug 12, contemporaneous with the commits), --no-build:

• AcDream.Core.Net.Tests — 877 passed / 0 failed / 0 skipped
• AcDream.Core.Tests — 4,676 passed / 0 failed / 1 skipped
• FA1-family filter (Allegiance|Fellowship|Confirmation|SocialActions) — 81 passed / 0 failed

Those two project totals are the anchors a corrected ledger row can be rebuilt from.

---

SHOULD-FIX

SF-1 — "UNWIRED / pure parse functions" is not true of ParseAllegianceInfoResponse

6bedbc47's commit message and the GameEvents.cs banner both frame FA1 as "pure parse functions + typed records only, UNWIRED". That holds for everything except the one function FA1 restructured that is live production today: ClientCommandResponses.ParseAllegianceInfoResponse is registered at src/AcDream.Core.Net/GameEventWiring.cs:192-198 and feeds FormatAllegianceInfoLines → chat for the shipped @allegiance info command.

FA1 changed its observable output in two ways:

1. Vassal print order flipped. FindVassals now walks the record list in reverse (ClientCommandResponses.cs:257-259), and FormatAllegianceInfoLines consumes it directly at :605. A player with 3+ vassals sees them listed in the opposite order to yesterday's build.
2. Malformed-tree output went from partial to silent. An orphan / self-parent / duplicate treeParent now returns null for the whole message (:509-516), and the wiring's if (info is null) return; prints nothing at all where the pre-FA1 parser printed a roster.

I verified both against retail rather than taking them on trust, and both are correct (see VC-4/VC-5) — so these are fidelity fixes to a live command, not regressions. But they should be described as such, and the reviewer-facing framing should not say "unwired". Two consequences:

• The change deserves a line in FA1's ledger row naming @allegiance info as the one live surface FA1 moved, so a user-visible ordering change is not a surprise at the FA5 gate.
• The reversal is untested at the layer that shows it. The only format-level test with vassals, tests/AcDream.Core.Net.Tests/Messages/ClientCommandResponsesTests.cs:280-312 (ParseAndFormatAllegianceInfoResponse_PatronAndVassals_RendersFullTree), has exactly ONE vassal, so it cannot distinguish forward from reverse order. Add a two-vassal case that pins the reversed order through FormatAllegianceInfoLines, not just through FindVassals.

SF-2 — Tree assembly omits retail's _id != 0 rejection

AllegianceHierarchy::Add @0x005B6E90 rejects any record whose own _data._id is zero, on both the monarch branch and the child branch — and a rejected Add discards the whole message (UnPack @0x005B779D: if (Add(...) == 0) return 0;).

ReadAllegianceProfileBody implements the orphan, self-parent, and duplicate rules but not this one (ClientCommandResponses.cs:494-524): a record with CharacterId == 0 is accepted. ACE never emits a zero id in practice so this is inert today, but the block's own comment enumerates retail's discard conditions as if complete. One || record.CharacterId == 0 (plus the monarch check) closes it.

SF-3 — Retail zeroes the monarch's MayPassupExperience; FA1's new column does not

Immediately after the record loop, UnPack does (@0x005B77A7-0x005B77B3):

class AllegianceNode* m_pMonarch = this->m_pMonarch;
if (m_pMonarch != 0)
    AllegianceData::SetMayPassupExperience(&m_pMonarch->_data, 0);

FA1's ReadAllegianceData computes MayPassupExperience per record (ClientCommandResponses.cs:578-579) including the legacy "HasPackedLevel absent ⇒ set it anyway" fallback, but nothing forces the monarch's back to false. The field is unsurfaced today so nothing observes it — but this is the one field whose comment explicitly argues fidelity ("a port that also sets the bit is MORE faithful than one that does not"), so the half of the retail rule that clears it belongs in the same commit. Cheapest place is in ReadAllegianceProfileBody right after the monarch record is read.

SF-4 — New doc comment cites a test class that does not exist

src/AcDream.Core.Net/Messages/GameEvents.cs:572-574 — "see ConfirmationResponseTests for the golden-vector / round-trip pair". The file is tests/AcDream.Core.Net.Tests/Messages/ConfirmationTripleTests.cs (class ConfirmationTripleTests). Trivial, but this is a citation in a file whose whole value is the accuracy of its citations.

SF-5 — Name the confirmation-triple discriminator's owner before FA4

Ownership is currently split, which is the latent FA4 landmine the review brief anticipated — though not in the shape it expected. There is no double parser: 0x0274/0x0276 had typed parsers before FA1 and FA1 did not touch them; 0x0275 is client-authored and never received, so FA1's new ParseConfirmationResponse competes with nothing. The problem is representational:

• FA1's new ConfirmationResponse.Type is the typed GameEvents.ConfirmationType (GameEvents.cs:577).
• The two legs actually consumed in production — CharacterConfirmationRequest.Type (:507) and CharacterConfirmationDone.Type (:530), read by GameplayConfirmationController.HandleRequest/HandleDone — remain bare uint.

So one triple now carries its discriminator two ways, and the typed one is on the leg nobody reads. Before FA4 wires fellowship (type 4) and allegiance (type 1) confirmations, either promote the two inbound records to ConfirmationType or state in the enum's doc comment that it is deliberately response-side only. Deciding now costs a sentence; deciding at FA4 costs a migration across RetailDialogFactory/GameplayConfirmationController.

---

Verified clean

Stated plainly, so the next reader does not re-walk these axes.

VC-1 — The renamed/reshaped builders have no surviving callers or fixtures. Repo-wide sweep for BuildFellowshipUpdate / BuildFellowshipCreate / FellowshipUpdateOpcode across src, tests, tools, .claude, docs, in-repo memory/, and the persistent memory dir. Production callers: none (H.2 left them caller-less and that still holds). tools/ and .claude/: no hits at all. Persistent memory (~/.claude/projects/.../memory/): no hits. memory/project_session_2026_04_18.md:137 is a dated session ledger (historical record, correctly left alone). The wire-message catalog the brief flagged does not exist — docs/research/2026-06-04-wire-message-catalog.md is not in the tree (only the generator .claude/workflows/research-wire-message-catalog.js); the MEMORY.md index entry pointing at it is a pre-existing broken link, unrelated to FA1. docs/research/2026-07-29-wire-stack-audit.md:487,931 cites both the deleted file and the old builder shape, but that doc declares its own Base commit: b70b9832 in its header — a point-in-time snapshot, correctly not FA1's to rewrite.

VC-2 — The deletion is complete. No references to AllegianceTree / AllegianceNode / AllegianceMath remain in src/ (the one hit, ClientCommandResponses.cs:247, cites lane C's DELETE verdict on the file and is accurate as written). Nothing in docs/ISSUES.md. Nothing in AcDream.slnx (project-level only; no per-file entries). Both directories (src/AcDream.Core/Allegiance/, tests/AcDream.Core.Tests/Allegiance/) are gone rather than left empty. The only docs/architecture hit is MF-1 above. ChatChannelInfoTests.cs:28 is an unrelated test-name string coincidence, as the commit message says.

VC-3 — The repaired BuildFellowshipCreate is byte-correct against ACE, and the trailing-pad question is settled. PackString16L (SocialActions.cs:389-403) already pads the string record to a 4-byte multiple including its own u16 length, so 12 + name.Length is 4-aligned and appending the u32 keeps it aligned — the pad the old builder added after its two invented bools was never the string's pad. ACE reads exactly ReadString16L() then ReadUInt32() (GameActionFellowshipCreate.cs), and ACE.Common's ReadString16L skips the same CalculatePadMultiple(2 + length, 4) pad. The golden vector at SocialActionsTests.cs:65-83 ("Team", len 4, 2 pad bytes) is hand-derived and correct. The other three builders match ACE too: 0x0290 ReadUInt32() newLeaderID, 0x0291 ReadUInt32() != 0, 0x00A6 Convert.ToBoolean(ReadInt32()), and GameActionType confirms 0x0290/0x0291/0x00A6 verbatim.

VC-4 — The eleven version gates match retail exactly. Walked AllegianceHierarchy::UnPack @0x005B7520 line by line against ReadAllegianceProfileBody. Confirmed: recordCount/oldVersion are the low/high halves of one u32 (@0x005B754D, matching the ReadU16/ReadU16 pair); the < 6 branch skips a single legacy spokesperson u32 only when >= 1 (@0x005B759B); titles at OfficersTitlesAdded; the four counters at PoolsAdded; motd and motdSetBy together at MotdAdded; chatRoomID; bind point at Bindstones; allegianceName and nameLastSetTime together at AllegianceName; LockedState; ApprovedVassal. Against ACE's constant oldVersion = 0x000B every gate is taken, so the cursor walk is identical to the pre-FA1 unconditional read — the live @allegiance info field extraction is unchanged. Also confirmed the first record carries no leading treeParent (UnPack's if (ebp_1 != 0) guard at @0x005B776E).

VC-5 — Both tree-assembly rules are retail-verified, and ACE cannot trip the discard. AllegianceHierarchy::Add @0x005B6E90 requires Search(treeParent) non-null (orphan), parent->_data._id != _id (self-parent), and Search(_id) == 0 (duplicate) — and UnPack @0x005B779D returns 0 on any Add failure, i.e. discards the entire message. The prepend is literal: eax_10->_peer = _vassal; esi->_vassal = eax_10; (@0x005B6F35-0x005B6F38), which is what makes the sibling walk reverse. acdream's running knownIds set is equivalent to retail's Search-from-monarch because both contain exactly the nodes added so far. Separately I walked ACE's AllegianceHierarchyExtensions.Write for every topology it can emit (monarch-only; patron-is-monarch; deep patron; player-is- monarch): it always writes monarch → patron → self → vassals, so every treeParent is already in the tree when its child arrives — the discard path should never fire against ACE. Also checked the adjacent truncation risk and cleared it: AllegianceNode.TotalVassals is Vassals.Count (direct only), so recordCount always matches the records actually written.

VC-6 — No handler-lane collision, no double registration, no enum churn. src/AcDream.Core.Net/GameEventWiring.cs is not in the FA1 diff — the 0x027C lane at :192-198 and the 0x0274/0x0276 lanes at :202-218 are byte-identical to pre-FA1. GameEventType.cs is also untouched, and every opcode FA1 parses (0x0003, 0x0020, 0x00A3, 0x00A4, 0x01C8, 0x01C9, 0x01CA, 0x027A, 0x02BE, 0x02BF, 0x02C0) was already declared there with the right value — no additions, no duplicate values, no shadowing. The only registration touching anything FA1 changed is the single 0x027C one, and 0x0020 has no registration at all (correct: FA2's job).

VC-7 — Cross-host symmetry holds. git diff --name-only bb48d2c8..ee1124ca over src/AcDream.Headless, src/AcDream.App, src/AcDream.Runtime, src/AcDream.UI.Abstractions returns empty. The headless host's behavior cannot have moved: the only shared-path semantic change is ParseAllegianceInfoResponse (SF-1), which the headless host does not consume — its sole consumer is the chat-render registration in GameEventWiring.

VC-8 — The WeenieError commit is genuinely inert. Every changed line in src/AcDream.Core/Chat/WeenieErrorMessages.cs is a comment; Resolve()'s behavior is untouched (verified by filtering the diff to non-comment lines — the result is empty). The new [Theory] proves all 8 ids resolve to no text, matching the existing Format_0x051D_ReturnsNull_NoRetailCaseExists precedent.

VC-9 — No register row is owed by FA1 itself (beyond MF-1's correction). I audited the implementer's "everything is fidelity-fix or inert" claim against the two candidates the brief named:

• D5 (shareLoot defensive parse). The plan's D5 promises "one register row records the knowing display divergence vs ACE's .3-at-nine-fellows server math". FA1 only parses ShareLoot as a raw uint into an unsurfaced record field; no share percentage is displayed anywhere yet. The row is owed by the slice that renders the share column — FA4, per the plan's own wording ("display per D5" is FA4 scope at :118). Correctly not filed now.
• D9 (the WeenieError outcome). No row: the register tracks acdream-vs-retail deviation, and FA1's finding is that acdream already matches retail (both silent). That two of the ids are on ACE's live send paths is an ACE-vs-retail difference, which the register does not track. Correctly not filed. (The plan text that this finding contradicts is MF-3.)

---

Coverage note

Every lens in the brief was traversed. Item 1 (builder-rename sweep) → VC-1 + MF-2. Item 2 (deletion completeness) → VC-2 + MF-1. Item 3 (live @allegiance info behavior) → VC-4/VC-5 + SF-1 (not bit-identical: two retail-faithful but user-visible changes). Item 4 (GameEvents.cs additions vs the 0x027C lane) → VC-6, clean. Item 5 (confirmation-triple ownership) → VC-6 + SF-5 (no double parser; a representational split instead). Item 6 (test accounting) → MF-4. Item 7 (register/ISSUES/plan bookkeeping) → MF-1, MF-3, VC-9. Item 8 (cross-host symmetry) → VC-7, clean.

No finding in this document is speculative: each cites a file:line, a retail address, or an ACE source file that I read.