docs: FA1 blast review -- APPROVE-WITH-FIXES (4 MUST-FIX, 5 SHOULD-FIX)

Blast-radius lens over Campaign FA slice FA1 (7be86f47, 6bedbc47, 5f9aa16f, 4281750b, ee1124ca). The wire work is right -- both repaired builders re-derived against ACE's own handlers, all eleven allegiance version gates against AllegianceHierarchy::UnPack @0x005B7520, and both tree-assembly rules against Add @0x005B6E90. No handler-lane collision, no double registration, no cross-host source touched. The bookkeeping is not. MUST-FIX: register row AP-90 still cites the deleted AllegianceTree; lane D's seam map -- FA2's own contract -- is falsified in four places including a row that would make FA6 re-introduce the exact openness/panel-visibility bug FA1 fixed; plan decision D9 still says the 8 WeenieError strings were added when FA1 shipped the opposite finding; and the ledger's test totals state three mutually exclusive numbers (+46 implied, +50 stated, +49 measured from the diffs). SHOULD-FIX: the "UNWIRED" framing is untrue of ParseAllegianceInfoResponse (live behind @allegiance info -- vassal print order now reverses and a malformed tree now silences the command outright; both retail-faithful, neither pinned by an order-sensitive test); the id != 0 discard rule is missing; retail's monarch MayPassupExperience zeroing is missing; one new doc comment cites a nonexistent test class; and the confirmation triple now carries its discriminator two ways. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-08-12 00:26:59 +02:00 · 2026-08-12 00:26:59 +02:00 · c4b7247484
commit c4b7247484
parent ee1124ca5b
1 changed files with 378 additions and 0 deletions
--- a/docs/research/2026-08-12-fa1-review-blast.md
+++ b/docs/research/2026-08-12-fa1-review-blast.md
@ -0,0 +1,378 @@
+# 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.