From c4b7247484abc544df6b070a7e583d7aeff86da5 Mon Sep 17 00:00:00 2001 From: Erik Date: Wed, 12 Aug 2026 00:26:59 +0200 Subject: [PATCH] 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 --- docs/research/2026-08-12-fa1-review-blast.md | 378 +++++++++++++++++++ 1 file changed, 378 insertions(+) create mode 100644 docs/research/2026-08-12-fa1-review-blast.md diff --git a/docs/research/2026-08-12-fa1-review-blast.md b/docs/research/2026-08-12-fa1-review-blast.md new file mode 100644 index 00000000..6693791e --- /dev/null +++ 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.