# 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.