diff --git a/docs/research/2026-08-12-fa5-review-mechanism.md b/docs/research/2026-08-12-fa5-review-mechanism.md new file mode 100644 index 00000000..f8c3caf8 --- /dev/null +++ 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.