docs(fa5): mechanism-faithfulness review — APPROVE-WITH-FIXES (1 SHOULD-FIX)

FA5 mechanism-faithfulness review of 7ed79eaf/bc29a1db/7e394cbf. Verdict APPROVE-WITH-FIXES. Every high-stakes claim re-derived from the PDB-paired 2013 decomp: CF-1's unconditional 0x001F post-world arm (00490d59 sits OUTSIDE the busy-count guard), the monarch/patron/self field sources (UpdatePlayerData/UpdateMonarchData/UpdatePatronData), the SF-7 per-relationship gate, swear=world-selection/no-SetSelectedObject, and the AD-86 ACE-zeroed-field citations all match retail. MANDATORY live-mount probe RAN and PASSED against the real installed DATs (1/1) — the scoped doubled-0x10000492 NotSame assertion and a full production Bind() with zero "not found" held. FA5 unit suite 36/36 green. One SHOULD-FIX (LOW): FA5 greys the offline vassal NAME (OfflineNameColor) — retail's UpdateVassalsData @004924c3 sets the name with no color; the offline cue is exclusively the authored 0x100004AA marker toggle. Either drop OfflineNameColor or honestly register it (the AD-82 addendum's "covered by the marker" framing understates it). Does not block the gate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-08-12 09:02:01 +02:00 · 2026-08-12 09:02:01 +02:00 · f12aefe948
commit f12aefe948
parent b6c4a4fa3a
1 changed files with 347 additions and 0 deletions
--- a/docs/research/2026-08-12-fa5-review-mechanism.md
+++ b/docs/research/2026-08-12-fa5-review-mechanism.md
@ -0,0 +1,347 @@
+# Campaign FA slice FA5 — mechanism-faithfulness review
+
+**Reviewer lens:** mechanism-faithfulness (retail-fidelity), read-only.
+**Date:** 2026-08-12. **Target commits:** `7ed79eaf` (code+tests),
+`bc29a1db` (register/gate-script/ledger), `7e394cbf` (NUL-byte repair).
+**Contract:** `docs/plans/2026-08-11-fellowship-allegiance-campaign.md`
+§FA5 (CORRECTED row — `0x001F` subscription, NOT `0x027B`) + lane-C
+allegiance-wire doc + `fa-panel-structure` §3.2/§10.
+**Worktree HEAD at review:** `b6c4a4fa` (clean).
+
+---
+
+## VERDICT: APPROVE-WITH-FIXES (one SHOULD-FIX, LOW severity)
+
+FA5 is mechanism-faithful. Every high-stakes claim was re-derived from
+the PDB-paired 2013 decomp (not trusted from the implementer's summary):
+CF-1's unconditional `0x001F` post-world arm, the monarch/patron/self
+field sources, the per-relationship SF-7 gate, the swear/break/kick
+targets, and the ACE-zeroed-field honesty all match retail. The
+mandatory live-mount probe **PASSED against the real installed DATs**
+(the anti-fixture-green safeguard the implementer could not run) —
+including the doubled-`0x10000492` scoped dual-resolution that is exactly
+where a live-only defect could have hidden.
+
+The single SHOULD-FIX is a LOW-severity invented-visual: FA5 greys an
+offline vassal's **name** (`OfflineNameColor`), which retail does not do
+— retail's offline indication is exclusively the authored `0x100004AA`
+marker toggle, with the name left at normal color. The AD-82 addendum
+mentions `OfflineNameColor` but frames it as "already covered by the
+retail-authored `0x100004AA` marker," which understates it: the
+name-grey is an *additional* cue retail never paints. This does not block
+the connected gate (data + the authored marker are correct); it needs
+either removal or an honest register entry.
+
+---
+
+## Live-mount probe — actual output (MANDATORY, reported explicitly)
+
+Run: `ACDREAM_PROBE_LIVE_MOUNT=1`,
+`ACDREAM_DAT_DIR=%USERPROFILE%\Documents\Asheron's Call`,
+`dotnet test tests/AcDream.App.Tests -c Release --no-build --filter
+"FullyQualifiedName~SocialPanelLiveMountProbe"`. The App test project was
+rebuilt first (with dependencies) since this fresh worktree's artifacts
+predated FA5.
+
+**Result: `Passed! - Failed: 0, Passed: 1, Skipped: 0` (exit 0).**
+`SocialPanelLiveMountProbeTests.ProbeLiveMountShapes [84 ms]`.
+
+The probe is NOT a no-op green — against live DATs it asserts (all held):
+
+- `Assert.Equal(2, passupCount)` — `0x10000492` occurs exactly twice
+  under the allegiance page.
+- The **scoped dual-resolution**: `monarchScopedPassup` found under
+  `0x10000490` (under monarch field `0x10000255`), `patronScopedPassup`
+  found under patron field `0x1000025A`, both non-null AND
+  `Assert.NotSame` — proving the two `FindDescendant` sites resolve to
+  DIFFERENT real instances. A flat/unscoped lookup would silently pick
+  one for both roles; this catches it.
+- Vassal ListBox `0x10000260` template resolves via the production
+  `RowTemplateResolver`; row fields `0x10000268`/`0x10000269` (UiText),
+  `0x100004AA` (offline marker) all resolve.
+- Checkbox `0x10000262` (UiButton) + label
+  `ID_PlayerOption_IgnoreAllegianceRequests` resolves non-empty.
+- All five allegiance strings resolve non-empty:
+  `ID_Allegiance_MonarchLabel`, `PatronSlashMonarchLabel`,
+  `SwearConfirmation`, `BreakConfirmation`, `KickConfirmation`.
+- A full production `SocialAllegiancePageController.Bind()` against the
+  live layout emits **zero "not found"** console warnings.
+
+The raw dump confirms `0x10000490` is authored under BOTH the monarch
+field (`0x10000255`, at `(210,0 90x36)`) and the patron field
+(`0x1000025A`, at `(220,0 80x36)`); FA5's monarch-experience lookup
+scopes to the monarch instance, and its patron-experience lookup recurses
+from the patron field (finding the patron instance) — matching retail's
+own `GetChildRecursiveTemplate(sub-block, …)` vs
+`GetChildRecursive(m_pPatronField, …)` split (see item 2).
+
+FA5 unit suite (targeted, this build):
+`SocialPanelControllerTests` + `GameplayConfirmationControllerTests` =
+**36 passed / 0 failed**.
+
+---
+
+## Per-item verification (re-derived from primary source)
+
+### 1. CF-1 — the `0x001F` subscription (highest stakes) — VERIFIED
+
+**Unconditional post-world arm (the core claim).** `pseudo-c:157175`,
+`gmAllegianceUI::RecvNotice_PlayerDescReceived @0x00490D40`:
+
+```
+if (m_bAwaitingUpdate == 0) { m_bAwaitingUpdate = 1; IncrementBusyCount(); }
+CM_Allegiance::Event_UpdateRequest(1);   // 00490d59 — OUTSIDE the if
+```
+
+The `Event_UpdateRequest(1)` at `00490d59` sits OUTSIDE the `if` guard
+(which only wraps the busy-count increment), so retail sends `0x001F(1)`
+**unconditionally**, exactly as the implementer claims.
+`SocialAllegiancePageController.RedeclareAfterWorldEntry()` mirrors this:
+it calls `SetUpdateSubscription(true)` with no latch comparison, and
+advances the latch only on `Accepted` (so a dropped send retries).
+
+**Three arming points, all confirmed against the decomp** (every
+`Event_UpdateRequest` site enumerated):
+`PostInit @0x004911C6 → 1` (Bind's `SetPageVisible(true)` attempt,
+pre-world no-op in production), `OnVisibilityChanged` visible
+`@0x004912DD → 1` / hidden `@0x00491311 → 0` (edge-triggered
+`SetPageVisible`), `PlayerDescReceived @0x00490D59 → 1`
+(`RedeclareAfterWorldEntry`, unconditional). A fourth retail arm exists —
+`OnQualityChanged @0x00490DC9 → 1` (Patron/Monarch instance-id change,
+`fa-panel-structure` §6.3) — NOT modeled by FA5; covered instead by
+acdream's `0x0020`-seed + `Revision` polling. Given the subscription
+model (data streams while subscribed), this is a benign
+data-refresh-mechanism substitution, not a functional gap.
+
+**Genuinely avoids MF-3-REOPEN.** Reconnect seams traced end-to-end:
+pre-world `LiveSessionResetBindings.SessionDialogs → ResetSessionTransientUi
+→ SocialPanelController.ResetSessionDeclaration →
+_allegiance.ResetPageVisibleLatch()` (clears latch, **no send**);
+post-world `EnteredWorld.RestoreLayout →
+RedeclareSocialPanelAfterWorldEntry →
+SocialPanelController.RedeclareAfterWorldEntry →
+_allegiance.RedeclareAfterWorldEntry()` (unconditional send). So a
+reconnect while the panel is CLOSED still re-subscribes.
+
+**The test genuinely exercises the world gate.**
+`Reconnect_ReDeclaresSubscription_AfterWorldEntry_EvenWhilePanelClosed`
+binds with the window never shown (`_visible=false`), clears the latch,
+then asserts `RedeclareAfterWorldEntry` records `subscription:True`. This
+only passes because the path is unconditional — a conjunction-gated shape
+(`UpdateAllegiancePageVisibility`) would compute `false && … = false`,
+and with the latch already `false` would send nothing, failing the
+assertion. Confirmed it would fail against an edge-triggered shape.
+
+**`0x027B` is NOT wired for data.** FA5's sole subscription surface is
+`SetUpdateSubscription` (`0x001F`); no info-request send exists in the
+controller. Class doc and gate-script both state `0x027B`/`0x027C` are
+text-only chat (FA2 MF-2). Confirmed.
+
+### 2. Monarch/patron/self blocks (SF-7 rewrite) — VERIFIED (all field sources)
+
+Re-decompiled `UpdatePlayerData @0x00491330`, `UpdateMonarchData
+@0x00491B40`, `UpdatePatronData @0x004917C0` fresh:
+
+- **`0x10000251` = the ALLEGIANCE name, not the viewer's.** `00491397`:
+  `m_pAllegianceName ← _allegiance.m_AllegianceName` via
+  `AddVariable_String`. FA5 `RefreshSelfBlock` uses
+  `snapshot.AllegianceName`. ✓
+- **Self followers `0x10000252` = `_total_vassals`.** `00491506`:
+  `AddVariable_Int(_total_vassals) → m_pPlayerFollowers`. FA5
+  `snapshot.TotalVassals`. ✓
+- **Self rank `0x10000253` = `InqInt(qualities, 0x1e)` (AllegianceRank)
+  + `GetTitle`.** `00491578`: `CBaseQualities::InqInt(&var_1f8[0xe],
+  0x1e, …)` then `AllegianceData::GetTitle`. FA5 substitutes
+  `snapshot.Rank` (documented data-source substitution). ✓
+- **Monarch gate = hide when `monarch==viewer || monarch==0`.**
+  `00491c0f`. FA5 `hasMonarch = monarch is {} m && m.CharacterId !=
+  selfGuid` (null when no monarch). Matches under all cases. ✓
+- **Hidden branch blanks name AND followers.** `00491f8c`
+  (`m_pMonarchName ← ""`) and `00491fcd` (`m_pMonarchFollowers ← ""`).
+  FA5 blanks both. ✓
+- **Shown followers = `_total_members - 1`.** `00491eac`:
+  `ecx_43 = ebx_3->_total_members - 1`. FA5 `TotalMembers - 1` (guarded).
+  ✓ (test asserts "2" for `TotalMembers=3`).
+- **patron==monarch → `PatronSlashMonarchLabel` + reveal `0x10000490`
+  sub-block + `0x10000492` = viewer's CpTithed.** `00491c60` (else of
+  `GetPatron != monarch`): sets label, `GetChildRecursive(0x10000490)
+  ->SetVisible(1)` `00491cbc`, `GetChildRecursiveTemplate(sub-block,
+  0x10000492)` with `GetData(viewer).cp_tithed`. FA5 maps
+  `patronIsMonarch → PatronSlashMonarchLabel`, `subBlock.Visible=true`,
+  `monarchExperiencePassedUp ← Member(self).CpTithed`. ✓ patron!=monarch
+  → `MonarchLabel` + hide sub-block (`00491dc5`). ✓
+- **Enabled ← IsLoggedIn** (`SetState(1)` vs `SetState(0xd)` `00491f0c`).
+  FA5 `_monarchField.Enabled = monarchData.IsLoggedIn`. ✓
+- **Patron gate = hide when `patron==0 || patron==monarch`.**
+  `004918b1`. FA5 `hasPatron = patron is {} p && (monarch is not {} m ||
+  p.CharacterId != m.CharacterId)`; negation matches retail in all four
+  cases (verified case-by-case). ✓ Shown: name = `GetFullName`
+  (`004918e0`); Enabled ← IsLoggedIn (`0049197d`); experience
+  `GetChildRecursive(m_pPatronField, 0x10000492)` = viewer's own tithed
+  (`004919b9` `GetData(viewer)`). FA5 matches (`patronExperiencePassedUp
+  ← Member(self).CpTithed`). ✓
+
+The doubled-`0x10000492` scoping matches retail exactly: monarch's under
+the sub-block, patron's recursively under the patron field. FA5's two
+`FindDescendant` sites (`Bind`, lines 315-319) reproduce this precisely.
+
+### 3. Vassal list — VERIFIED
+
+`RuntimeAllegianceState.AllegianceView.GetVassals` returns
+`AllegianceProfileLookups.FindVassals` order — the FA1-assembled
+REVERSE-wire order (each record prepended, doc comment cites lane C
+§4.4). FA5 `RefreshRoster` materializes `new List<>(Vassals(self))`
+and iterates with **no re-sort**; `RebuildRoster` uses
+`FlushPreservingScroll()` (NOT `Flush()`), so the FA3 scroll-reset
+carry-forward is NOT reintroduced. The allocating `GetVassals` call runs
+only inside the `snapshot.Revision != _lastRosterRevision` gate — not
+per-frame (honoring FA2's blast SF-7 caution). ✓
+
+### 4. Swear/break/kick targets + type-1 confirmation — VERIFIED
+
+- **Swear = WORLD selection.** `UpdateSwearButton @0x004908E0` gates on
+  `GetPatron(self)==0 && selectedID!=0 && selectedID!=self &&
+  IsPlayer(selectedID) && GetData(selectedID)==0`, where
+  `ACCWeenieObject::selectedID` is the world selection. FA5 reads
+  `Selection.SelectedObjectId` at click, resolves via
+  `ResolveWorldObjectName` (= `Objects.Get(guid)?.GetAppropriateName()`,
+  the same `ClientObjectTable` resolver `ToolbarRuntimeBindings.ResolveName`
+  uses). ✓ The one omission — `IsPlayer()` — is registered AD-84 (same
+  class as AD-83). ✓
+- **No SetSelectedObject for Allegiance's list** (fa-panel-structure
+  §6.2): Allegiance's `ListenToElementMessage` idMessage-4 reads the row's
+  `0x10000001` into `m_iidSelectedVassal` only — no
+  `ACCWeenieObject::SetSelectedObject` (Fellowship's DOES call it). FA5's
+  panel-local `_selectedVassalGuid` with no world-sync is faithful, and
+  the FA4 world→panel sync is NOT reintroduced for Allegiance. ✓
+- **Break = current patron** (`UpdateBreakButton @0x004909D0`:
+  `GetPatron!=0`; `MakeBreakConfirmationDialog @0x00492BF0`). FA5
+  `OnBreakClick → Patron(self) → Break(patron.CharacterId)`. ✓
+- **Kick = panel-local selected vassal** (`MakeKickConfirmationDialog
+  @0x00492E10`, `m_iidSelectedVassal`). FA5 `OnKickClick →
+  _selectedVassalGuid → Kick`. ✓
+- **Type-1 confirmation reaches the generic controller.**
+  `RetailUiRuntime.HandleConfirmationRequest` forwards every request
+  straight to `GameplayConfirmationController` (no intercept — FA4's
+  type-4 removal never touched type-1). New test
+  `AllegianceSwearRequest_Type1_OpensDialog_MessageVerbatim_AndSendsAcceptOnClose`
+  verifies a `ConfirmationType 1` opens a verbatim dialog and sends
+  accept on close. ✓
+
+### 5. ACE-zeroed fields (AD-86) — VERIFIED (citations accurate)
+
+`references/ACE` is not checked out in this worktree, so lane C §5.1 is
+the authority. AD-86's cited lines match it field-for-field:
+`AllegianceHierarchy.cs` officers `:74-75`, officerTitles `:78-83`,
+motd/motdSetBy `:86-89`, nameLastSetTime/isLocked/approvedVassal
+`:62-64`/`:153-155`, broadcast counters `:53-56`; `AllegianceData.cs`
+timeOnline/allegianceAge `:59-60,86-89,111-112`. Retail's own
+`gmAllegianceUI` has no widget for any of them (fa-panel-structure §3.3),
+so dropping them past the parse layer matches retail's presentation. The
+register row, blast column, and lane-C pointer are all accurate.
+
+### 6. §6 DISCIPLINE compliance — VERIFIED
+
+- **Resolver on every Build.** Main panel Build passes `resolver.Resolve`
+  (`RetailUiRuntime.cs:2715`); the vassal row `RowTemplateResolver`
+  builds each template with `strings.Resolve` (`:2765`). ✓
+- **Doubled `0x10000492` scoped under each parent** (both sites found):
+  monarch under `monarchIsPatronSubBlock` (`0x10000490`), patron under
+  `patronField` — probe-confirmed `NotSame`. ✓
+- **No lazy `0x0`.** All `FindDescendant` failures null-check + log; no
+  magenta-placeholder id path. ✓
+- **`BlankSentinel` is compare-only and never displayed.** Used solely in
+  `SetProvider`'s `lastValue == sentinelValue` / `lastValue =
+  sentinelValue`; the rendered text comes from `BlankLineProvider`
+  (`[new UiText.Line(" ", …)]`, a literal single space). The NUL-repair
+  commit `7e394cbf` restored the two spaces; **byte-swept all 11
+  FA5-touched files (src/tests/docs): NUL=0, control-char=0.** ✓
+
+### 7. Retail anchors + gate-script §FA5 — VERIFIED
+
+Five+ anchors spot-checked and confirmed: `Event_UpdateRequest` site
+inventory (`157175/157212/157414/157455/157495`); `UpdateSwearButton`
+enable rule; `UpdateBreakButton`; `UpdateVassalsData @0x00492340`
+(offline marker `SetVisible(1)` when `!IsLoggedIn` `@004925b1`; row
+identity via `SetAttribute_InstanceID(0x10000001)`); the `Update()`
+ordering (self→monarch→patron→vassals→swear→break→kick,
+`pseudo-c:158677+`) matches FA5's `Tick`.
+
+The gate-script §FA5 **correctly avoids the false-route the reviewer
+flagged**: it explicitly states the panel populates via `0x001F`→`0x0020`
+on tab-show, and says "**Do not use `@allegiance info` as a trigger for
+anything in this section.**" The FA3 section (already corrected in FA3's
+fix round) is consistent — it tells the user FA3 does NOT send `0x001F`
+and `@allegiance info` won't reveal the panel. Step 4 correctly describes
+the unconditional post-world resend as the MF-3-REOPEN analogue; steps
+5-8 pre-empt the AD-85 "just a number" and SF-7 per-relationship false
+reports. No wrong step.
+
+---
+
+## SHOULD-FIX (numbered)
+
+**SF-1 (LOW) — offline vassal name-grey is an invented visual not in
+retail; register or remove it.** `UpdateVassalsData @0x004924c3` sets the
+vassal name (`0x10000268`) via `GetFullName` with **no color argument**;
+retail's *only* offline indication is the dedicated `0x100004AA` marker
+toggle (`@004925b1`: `SetVisible(1)` offline / `(0)` online).
+`SocialAllegiancePageController.UpdateRow` (`SocialAllegiancePageController.cs:708`)
+additionally renders the offline name in `OfflineNameColor
+(0.6,0.6,0.6,1)` — a grey retail never paints. The AD-82 FA5 addendum
+mentions `OfflineNameColor` but frames it as "already covered by the
+retail-authored `0x100004AA` marker," which understates it: the marker is
+faithful; the name-grey is a *separate additional* cue on top of it.
+**Fix:** either (a) render vassal names in `TextColor` unconditionally
+(drop `OfflineNameColor`), letting the authored `0x100004AA` marker carry
+the offline state exactly as retail does — the more faithful option — OR
+(b) correct the AD-82 addendum to honestly register the offline
+name-grey as an adaptive cue rather than claiming no invented tints are
+used. Does not block the connected gate (data + marker are correct).
+
+---
+
+## Minor observations (NOT fixes — no action required)
+
+- **`OnQualityChanged`/quality-handler re-subscription arms** (Patron
+  `0x19`/Monarch `0x1A` instance-id change → `Event_UpdateRequest(1)`,
+  `00490dc9`, fa-panel-structure §6.3) are not modeled. Benign: acdream's
+  subscription-plus-`0x0020`-revision-polling refreshes the data through
+  a different mechanism. Not a functional gap.
+- **`SetTalkFocusEnabled(4/5/6, …)`** (patron/monarch/vassal chat-focus
+  toggles, tail of each Update* function) are not modeled — a
+  chat-integration nicety outside the panel-display scope.
+- **Monarch label `LinesProvider` is reassigned unconditionally each
+  `Tick`** in the shown branch (`RefreshMonarchBlock:555`) — a per-frame
+  closure/list allocation, unlike the guarded `SetLine` used for
+  name/followers. Blast-lens perf nit (rarely changes; could be guarded);
+  not a mechanism concern.
+- **NIT:** AD-86's prose says "seven fields" but the parenthetical lists
+  nine (7 `AllegianceProfile` + 2 `AllegianceData`). Cosmetic; the field
+  list and citations are complete/correct.
+
+---
+
+## What verified clean (explicit)
+
+- **Live-mount probe: PASSED against real DATs** (1/1), including the
+  scoped doubled-`0x10000492` `NotSame` assertion and full production
+  `Bind()` with zero "not found".
+- CF-1's unconditional post-world `0x001F` arm is byte-faithful to
+  `RecvNotice_PlayerDescReceived` and genuinely closes MF-3-REOPEN; the
+  decisive reconnect test exercises the world-closed gate.
+- All monarch/patron/self field sources match a fresh decompile of
+  `UpdatePlayerData`/`UpdateMonarchData`/`UpdatePatronData`.
+- Vassal roster: FA1 reverse order preserved, no re-sort,
+  `FlushPreservingScroll` (no scroll-reset), revision-gated (not
+  per-frame).
+- Swear=world-selection / Break=patron / Kick=panel-local; no
+  `SetSelectedObject` for Allegiance; type-1 confirmation flows to the
+  generic controller.
+- AD-84/AD-85/AD-86 + AD-82 addendum are accurate (AD-86's ACE citations
+  match lane C §5.1 line-for-line).
+- §6: resolver on every Build; doubled ids scoped; no lazy `0x0`;
+  `BlankSentinel` compare-only; **zero NUL/control-char corruption across
+  all 11 FA5 files** after `7e394cbf`.
+- Gate-script §FA5 states the true `0x001F`-not-`@allegiance-info`
+  expectation — no false-route.