From 913e35cdb5baceba1621c8855c7836c153587619 Mon Sep 17 00:00:00 2001 From: Erik Date: Wed, 12 Aug 2026 05:01:53 +0200 Subject: [PATCH] docs: FA4 mechanism review -- APPROVE-WITH-FIXES (5 MUST-FIX, 8 SHOULD-FIX) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Findings persisted before any fixer dispatch, per the campaign's §7 review protocol. MUST-FIX, in the order they were derived: 1. D5's percentage conversion rounds where retail truncates. Byte-decoded 0x0048ECC9..0x0048ECD8 (fld pct; fmul [0x007A5170]=100.0f; call _ftol2 @0x005DE394 -- the fld/fst/fistp/fild truncation dance), so a 6-fellow roster displays 45% where retail shows 44%, and 8 fellows shows 35% vs 34%. The table itself IS byte-exact; only the ->int conversion diverges, and it is not fixable by a plain cast because 0.45f*100f already rounds up to 45.0f in single precision. 2. D6's client-side invite intercept has no retail anchor. Read in full: Handle_Character__ConfirmationRequest @0x005640A0 (bare jump table), RecvNotice_FellowshipRequest @0x00490880, MakeFellowRequestDialog @0x00490620 (only guard is m_fellowRequestContext), plus a whole-file sweep of both option accessors -- zero reads on any confirmation path. The code comment cites ACE's Fellowship.cs as "retail". ACE filters both bits server-side, so the intercept is dead against a correct server and harmful against a drifting one -- and IgnoreFellowshipRequests defaults to TRUE client-side. 3. D4 never re-declares 0x00A6 after a generation reset: the edge- triggered _pageVisible latch survives reconnect, so fellow vitals stay frozen for the whole new session. ResetSessionTransientUi is the seam. 4. gmFellowshipUI::UpdateFellowSelection @0x0048F0F0 is not ported -- selecting a fellow in the WORLD leaves Dismiss/Leader disabled and no row ever shows selected; the plan's contracted UiTemplateListBox selection model + 0x1000000D row instance-id were not added. 5. Three shipped deviations have no register row (invite intercept, gold leader tint, name-text-only row selection); the Recruit is-a-player gate's "inline comment, not a register row" call is also wrong. Re-derived rather than trusted: the live-mount probe was re-run against the installed DATs (every ledger element/string claim CONFIRMED, Bind() warning-free), the GetEvenSplitXPPctg table was byte-read from the PDB-paired binary, FlushPreservingScroll's shrink semantics were traced through UiScrollablePanel/UiScrollable (sound), and the five touched test classes pass 93/93. Co-Authored-By: Claude Fable 5 --- .../2026-08-12-fa4-review-mechanism.md | 703 ++++++++++++------ 1 file changed, 472 insertions(+), 231 deletions(-) diff --git a/docs/research/2026-08-12-fa4-review-mechanism.md b/docs/research/2026-08-12-fa4-review-mechanism.md index 1bc316e3..e0f03b95 100644 --- a/docs/research/2026-08-12-fa4-review-mechanism.md +++ b/docs/research/2026-08-12-fa4-review-mechanism.md @@ -1,263 +1,504 @@ -# FA4 — mechanism-faithfulness review +# Campaign FA slice FA4 — MECHANISM-FAITHFULNESS review -**Slice:** Campaign FA slice FA4 — the Fellowship page fully live. -**Targets:** `357d2032` (Runtime), `5bdd0528` (App+tests), `38f08314` (docs). -**Reviewer lens:** mechanism-faithfulness (does the code do what retail does, -byte-for-byte where a byte exists, and is every divergence honestly filed). -**Method:** re-derived from the code, the lane docs -(`2026-08-11-fa-fellowship-wire.md`, `-fa-panel-structure.md`), the -PDB symbol table (`docs/research/named-retail/symbols.json`), and a LIVE run -of the env-gated mount probe against the real installed DATs. +**Reviewer lens:** mechanism faithfulness (does the shipped code do what +retail does, for the reasons retail does it). Sibling lens: regression / +blast radius (separate doc). + +**Target:** `357d2032` (Runtime), `5bdd0528` (App + tests), `38f08314` +(docs) against `docs/plans/2026-08-11-fellowship-allegiance-campaign.md` +slice FA4 + D4/D5/D6/D7 + the four FA3 re-review carry-forwards, with +`docs/research/2026-08-11-fa-fellowship-wire.md` (lane B) and +`-fa-panel-structure.md` (lane A) as ground truth. + +**Verdict: APPROVE-WITH-FIXES — 5 MUST-FIX, 8 SHOULD-FIX, 3 NIT.** + +The slice is real work and most of it is faithful: the button-enable +table is a verbatim port, the even-split *table* is byte-exact, the +roster diff genuinely never rebuilds on a vitals tick, the create-flow +gating matches retail's "the button IS the refusal mechanism", and the +live-DAT probe (re-run by this review, below) substantiates every +element-resolution claim in the ledger. The MUST-FIXes are not polish: +two of them are behaviors retail's binary demonstrably does *not* have +(D6's invite intercept, D5's rounding), one is a mechanism retail *does* +have and this slice did not port (world→panel selection sync), one is a +lifecycle hole that silently disables the very stream D4 exists to turn +on, and one is the register rule. --- -## VERDICT: APPROVE-WITH-FIXES +## 0. What this review re-derived (not trusted) -One MUST-FIX (a doc-only register omission), three SHOULD-FIX, two NITs. No -code-behavior REJECT: every wire-touching mechanism this slice ships is -retail-faithful and the divergences it makes are (with the one exception -below) honestly registered. - -**Strongest single piece of evidence:** the live-mount probe -(`ACDREAM_PROBE_LIVE_MOUNT=1`, real installed DATs, not a fixture) **PASSES** -(1/1). It confirms, end-to-end against the production mount, that the name -field builds as `UiField`, all 11 buttons/checkboxes resolve as `UiButton`, -the sole row template `0x21000030/0x10000281` resolves through the production -`RowTemplateResolver` with all five checked fields at the right widget types -(`UiText`/`UiText`/`UiMeter`/`UiMeter`/`UiMeter`), the four checkbox labels -resolve to real retail English, the Open/Close captions resolve to -`"Open"`/`"Close"`, and a full production-path `Bind()` emits zero -"not found" warnings. FA4 test suites also pass under `--no-build`: -`SocialFellowshipPageController`/`RowTemplateResolver`/`UiTemplateListBoxFlushPreservingScroll`/`CharacterOptions` -= 75/75; `RuntimeFellowshipStateTests` = 28/28; probe = 1/1. +| Claim | How it was checked | Result | +|---|---|---| +| Live-DAT mount/element/string claims in the FA4 ledger row | Re-ran `ACDREAM_PROBE_LIVE_MOUNT=1 dotnet test --no-build -c Release --filter SocialPanelLiveMountProbeTests` against the installed DATs | **CONFIRMED.** `0x1000026F` builds as `UiField`; all 11 buttons/checkboxes as `UiButton`; ListBox `0x10000279` templates=1 `0x21000030/0x10000281`, scrollbar `0x1000027A`; all 5 row fields at the right widget types; labels resolve ("Ignore Fellowship Requests", "Automatically Accept Fellowship Requests", "Share Fellowship Experience and Luminance", "Share Fellowship Loot"); captions resolve `'Open'`/`'Close'`; production `Bind()` console output **empty** | +| The frame-containment "structural finding" | Same probe dump | **CONFIRMED and EXTENDED** — `0x1000026B` children are `0x10000272,0x10000273,0x10000274,0x1000026C..0x10000271`: the Create button, the name field **and all four checkboxes** live in the NOT-in-fellowship frame. See SHOULD-FIX 8 for the consequence the ledger missed | +| `GetEvenSplitXPPctg` table | Byte-read `.rdata` of the PDB-paired `acclient.exe` at the ten VAs lane B §7.2 names | **CONFIRMED verbatim**, including `0x007E72B8 = 0.31111109256744385` and default `0.0` | +| How retail converts that float to the displayed integer | Byte-decoded `0x0048ECC9..0x0048ECD8` + the callee at `0x005DE394` | **REFUTES the port** — see MUST-FIX 1 | +| Whether retail's client reads the two fellowship option bits on the invite path | Read `gmFellowshipUI::RecvNotice_FellowshipRequest @0x00490880`, `MakeFellowRequestDialog @0x00490620`, `ClientUISystem::Handle_Character__ConfirmationRequest @0x005640A0` in full + swept every `IgnoreFellowshipRequests`/`FellowshipAutoAcceptRequests` occurrence in the 2013 pseudo-C | **REFUTES D6's invite half** — see MUST-FIX 2 | +| The 93 tests in the five touched test classes | `dotnet test --no-build -c Release --filter` | 93 passed / 0 failed | --- -## What verified CLEAN (re-derived, not trusted) +## MUST-FIX 1 — D5's percentage conversion rounds where retail truncates: 6- and 8-fellow rows display the wrong number -### D4 — the `0x00A6` gate (the highest-stakes mechanism) -The five contracted transitions all resolve correctly. The conjunction is -`_fellowship?.SetPageVisible(_visible && IsShowingFellowship)` -(`SocialPanelController.cs:267-268`), driven from three sources: -`ActivePageChanged` (ctor `:140`), `OnShown` (`:250-254`), `OnHidden` -(`:258-262`). `SetPageVisible` sends `0x00A6` only on an actual flip -(`SocialFellowshipPageController.cs:453-458`, `if (_pageVisible == visible) return;`). +`SocialFellowshipPageController.cs:650` -- **open-on-fellowship:** `OnShown` → `_visible=true` ∧ `IsShowingFellowship=true` - → `SetPageVisible(true)` → one `true` send. ✓ -- **tab-away:** `ActivePageChanged` → `_visible=true` ∧ `false` - → `SetPageVisible(false)` (flip from true) → one `false` send. ✓ -- **tab-back:** → `SetPageVisible(true)` → one `true` send. ✓ -- **close-while-on-fellowship:** `OnHidden` → `_visible=false` - → `SetPageVisible(false)` → one `false` send. ✓ -- **close-while-on-other-tab:** `_pageVisible` already `false` (we left the - Fellowship tab earlier), `OnHidden` → `SetPageVisible(false)` → no-op, **no - redundant send**. ✓ -- **switch between two non-Fellowship tabs while shown:** - `SetPageVisible(false)` while already `false` → no spurious send. ✓ +```csharp +return $"{member.Level} {(int)MathF.Round(pct * 100f)}%"; +``` -The default authored tab is Allegiance (probe: `button=0x1000028C -page=0x10000291 default=True`), so `ActivateTabs()` at mount time -(`RetailUiRuntime.cs:2775`, before the window is shown with `Visible=false`) -fires `ActivePageChanged` for Allegiance while `_visible=false` → no spurious -`0x00A6` at mount. ✓ +The class doc and the commit message both claim "The percentage itself IS +retail's exact number". The *table* is exact (byte-verified this review). +The **conversion is not**. -**No-send-while-disconnected** verified at the command layer: `Invoke` -returns `RuntimeCommandStatus.Inactive` without dispatching when -`_deactivated || _view is null || _commands is null` -(`InteractionUiRuntimeSources.cs:181-186`); `FellowshipSetPanelOpen` just -forwards through it (`:157-159`). **Idempotence** verified twice (the flip -guard + the pinning test `SetPageVisible_SendsPanelOpen_OnlyOnATransition`, -`SocialFellowshipPageControllerTests.cs:537-554`). +Retail, byte-decoded from the PDB-paired binary at +`gmFellowshipUI::UpdateFellowStats`: -### Leader-quit rule (FA2 hand-off not bypassed) -FA4's Quit routes `_bindings.Quit(disband)` → -`late.GameRuntime.FellowshipQuit` → `commands.Fellowship.Quit` -(`InteractionUiRuntimeSources.cs:138-140`), whose adapter body sends -`0x0290 SendFellowshipAssignNewLeader` BEFORE `0x00A3 SendFellowshipQuit` -when `RequiresLeaderHandoffBeforeQuit` returns true -(`DirectGameRuntimeCommandAdapter.cs:888-895`, mirrored -`CurrentGameRuntimeCommandAdapter.cs:883`). No raw `WorldSession.Send*` -bypass exists in the UI layer. The FA2 rule itself is correct: fires only -when `!disband && _isInFellowship && _leaderGuid == selfGuid`, picks the -first non-self member (`RuntimeFellowshipState.cs:292-313`). ✓ +``` +0048ECC9 d9 44 24 10 fld dword [esp+0x10] ; pct (float) +0048ECCD d8 0d 70 51 7a 00 fmul dword [0x007A5170] ; = 100.0f (byte-read) +0048ECD3 e8 bc f6 14 00 call 0x005DE394 ; _ftol2 +``` -### D5 — the XP-share table -`EvenSplitPercentTable = [1.0, 0.75, 0.6, 0.55, 0.5, 0.45, 0.4, 0.35, -0.3111111, 0.28]` (`SocialFellowshipPageController.cs:103-104`) reproduces -lane B §7.2's byte-decoded `.rdata` floats **exactly**, including the -out-of-range default `0.0` (`EvenSplitPercent`, `:655-656`, returns `0f` for -`memberCount` outside `[1,10]`). Branch structure matches -`gmFellowshipUI::UpdateFellowStats` §2.9 verbatim (`FormatStatsText`, -`:643-653`): `!ShareXp` → `0%` (retail's literal `pct=0.0f` branch), -even-split → table, proportional → **omitted** (level only) rather than a -guessed formula — the honest, CLAUDE.md-compliant disposition. AD-80 -accurately states the ACE display divergence (`.3` vs `.3111111` at 9, no -10-fellow row, wrong `1.0` out-of-range default) as ACE-vs-retail, not -acdream-vs-retail. ✓ +`0x005DE394` is MSVC's `_ftol2` (`55 8b ec 83 ec 20 83 e4 f0 d9 c0 d9 54 +24 18 df 7c 24 10 df 6c 24 10 …` — the fld/fst/fistp/fild + correction +dance whose whole purpose is to turn the FPU's round-to-nearest `fistp` +into **C truncation toward zero**). The product is formed on the x87 +stack from two floats, so it is exact; `_ftol2` then chops it. -### D6 — invite auto-response -`HandleConfirmationRequest` intercepts type-4 (`ConfirmationType.Fellowship=4`, -`GameEvents.cs:580`) BEFORE the generic `GameplayConfirmationController` -(`RetailUiRuntime.cs:677-683`). `TryAutoRespondToFellowshipInvite` (`:685-693`) -reads both bits and sends `SendConfirmationResponse(type, ctx, autoAccept)` -(the shared confirmation-response primitive, -`InteractionRetainedUiComposition.cs:807-812`) — decline when only Ignore is -set, accept when AutoAccept is set. The mutual exclusion IS Runtime's: -`RuntimeCharacterState.TrySetOption` clears the opposite bit when either is -turned on (`:821-834`), so both-set is unreachable through the write path. -Neither bit set → `return false` → falls through unchanged. ✓ +Per-entry comparison (float bits read from the binary; acdream column is +the shipped expression): -### D7 — un-dims -Exactly four rows flip `StoreOnly`→`Live` -(`CharacterOptionsPageController.cs:206-208`): `IgnoreFellowshipRequests`, -`FellowshipAutoAcceptRequests`, `FellowshipShareXP`, `FellowshipShareLoot`. -Each has a live read site: the first two in -`TryAutoRespondToFellowshipInvite`; `ShareXP` at the Create-button click -(`SocialFellowshipPageController.cs:325`) and its own checkbox; -`ShareLoot` via its own live checkbox surface (`RefreshCheckboxSelections` -`:484-486`). `IgnoreAllegianceRequests`/`DisplayAllegianceLogonNotifications` -stay `StoreOnly` (`:206`/`:208`). Conformance test updated 35→31 store-only / -15→19 live with the exact four ids removed from `ExpectedStoreOnlyIds` -(`CharacterOptionsPageControllerTests.cs:729-733`, `:762-765`). ✓ +| n | retail float | retail `trunc(pct×100)` | acdream `(int)MathF.Round(pct*100f)` | +|--:|---|--:|--:| +| 1 | 1.0 | 100 | 100 | +| 2 | 0.75 | 75 | 75 | +| 3 | 0.6000000238418579 | 60 | 60 | +| 4 | 0.550000011920929 | 55 | 55 | +| 5 | 0.5 | 50 | 50 | +| **6** | **0.44999998807907104** | **44** | **45** ❌ | +| 7 | 0.4000000059604645 | 40 | 40 | +| **8** | **0.3499999940395355** | **34** | **35** ❌ | +| 9 | 0.31111109256744385 | 31 | 31 | +| 10 | 0.2800000011920929 | 28 | 28 | -### Roster scroll preservation -`FlushPreservingScroll` (`UiTemplateListBox.cs:290-296`) saves `ScrollY`, -`ClearContent()`s, restores `ScrollY`. It genuinely preserves across a -membership-change rebuild because `ClearContent` zeroes only the *panel's* -`ContentHeight` and never syncs `Scroll.ContentHeight` -(`UiScrollablePanel.cs:54-61`), so the restore's `SetScrollY` clamps against -the STALE (pre-rebuild) `MaxScroll` (`UiScrollable.cs:56`) — exactly as the -method's own doc claims. The next `LayoutScrollableChildren` re-clamps -against the fresh height. Tests exercise the real cases: preserve across -same-count rebuild, clamp-down on shorter content, dormant no-op -(`UiTemplateListBoxFlushPreservingScrollTests.cs:50-98`). The no-change path -(`UpdateRow`) only sets `LinesProvider`/meter delegates — no ListBox -mutation (`SocialFellowshipPageController.cs:607-627`). ✓ +Two of the ten roster sizes are wrong. Note the failure is not fixed by +swapping `MathF.Round` for a cast: `0.45f * 100f` **already rounds up to +exactly `45.0f`** in single precision, so `(int)(pct * 100f)` still +yields 45. The product must be formed in a wider type, exactly as +retail's x87 does: -### Create-flow refusal-by-enabled-state -`RefreshCreateButtonState` sets `Enabled = !IsNullOrWhiteSpace(name)` -(`:460-465`); the click handler re-guards (`:322-327`). Matches lane B §2.2 -(`gmFellowshipUI::CreateFellowship` @0x0048F730): the button IS the guard, -no separate error text. `ShareXP` read from the character option at click -time (lane B row 67: "shareXP comes from option 0x0F"), not a dialog field. ✓ +```csharp +return $"{member.Level} {(int)((double)pct * 100.0)}%"; +``` -### Button-enable rules (§2.8 verbatim) -`RefreshButtonStates` (`:510-537`) ports the `gmFellowshipUI::UpdateButtons` -@0x0048E6C0 table exactly: Quit always; Disband/Open `= isLeader`; -Leader/Dismiss `= isLeader && hasSelection && !selectedIsSelf`; Recruit -`= targetValid && notFull && (isLeader || IsOpen)`. The one gap (Recruit does -not gate on "target is a player") is honestly disclosed as a -superset-of-retail rule in an inline comment (`:529-534`) with the server -refusing a non-player target the same way retail's click-handler no-ops. +(`(double)0.44999998807907104 * 100.0 = 44.999998807907104` → 44 ✓; +`0.3499999940395355 * 100.0 = 34.99999940395355` → 34 ✓; every other row +unchanged.) -### §6 compliance / anchors -String resolver threaded to every `LayoutImporter.Build` -(`RetailUiRuntime.cs:2714-2720`, the `RowTemplateResolver` build closure); -scoped `FindDescendant(pageRoot,…)`/`FindDescendant(row,…)` throughout; -scrollbar id-0 guard (`:247-256`); `RowTemplateResolver` caches null misses -so a permanently-unresolvable template imports ONCE (`RowTemplateResolver.cs:63-72`), -and the revision latch advances before `RefreshRoster` -(`SocialFellowshipPageController.cs:433-437`) so a partial resolver failure -never becomes a per-frame retry. Five retail anchors byte-verified in the -PDB: `0x0048E6C0`→`gmFellowshipUI::UpdateButtons`, -`0x0048F730`→`::CreateFellowship`, `0x005B9070`→`Fellowship::IsFull`, -`0x005B9BA0`→`FellowshipSystem::GetEvenSplitXPPctg`, -`0x004901C0`→`gmFellowshipUI::ListenToElementMessage`, -`0x0048EB20`→`::UpdateFellowStats`, `0x006A6380`→`CM_Fellowship::Event_UpdateRequest` -— all match their citations. `SelectionChangeSource.Social` -(`SelectionState.cs:21-26`) is anchored to `::ListenToElementMessage`'s -`SetSelectedObject` call and matches the shared selection primitive. Gate -script §FA4 claims all trace to the code (create gating, caption swap, button -states, D5 display, invite bits, leader hand-off). ✓ +**Also fix in the same commit:** +- `SocialFellowshipPageControllerTests.FormatStatsText_MatchesD5Rules` + exercises only n=1/n=9/proportional — neither failing size is pinned. + Add `[InlineData(true, true, 6, "12 44%")]` and + `[InlineData(true, true, 8, "12 34%")]`. +- The gate script's §FA4 step 21 currently presents the 9-member ACE gap + (AD-80) as the *only* percentage divergence a tester should tolerate, + which would prime them to accept the wrong 45% at six fellows as + correct. Same class as FA3's own mechanism MF-1/MF-2 script + corrections. +- AD-80's row text says acdream "renders retail's own byte-decoded XP-share + table verbatim" — true of the table, false of the rendered integer until + this lands. --- -## Findings +## MUST-FIX 2 — D6's invite intercept is a client-side mechanism retail does not have, and it can silently swallow real invites -### MUST-FIX 1 — the leader-gold-tint adaptation has no divergence-register row -`SocialFellowshipPageController` tints the fellowship LEADER's roster name a -distinct gold (`LeaderNameColor = (1, 0.84, 0, 1)`, applied in `UpdateRow`, -`SocialFellowshipPageController.cs:110`/`:614`). Its own class doc -(`:57-65`) is honest that lane A's row template has **no** dedicated leader -marker and retail "may simply not mark leadership in the row at all" — i.e. -this is an *invented, user-visible visual cue retail (probably) does not -have*. That is precisely the class of deviation the divergence register -exists to hold, and CLAUDE.md's binding rule is unambiguous: "Any commit that -introduces a deviation … adds its register row IN THE SAME COMMIT" / "A -deviation found without a row is a bug twice over." The FA4 docs commit filed -AD-80 (XP-share display) and AD-81 (the numeric-composite text gap) but -**neither covers the gold tint** — a register grep for `gold`/`tint` returns -only the unrelated AP-178 (SpewBox). This is exactly the same reasoning that -earned AD-78 its row ("caption colors retail never has → register row"). Fix: -add a one-line AD row (or fold a clause into AD-81) recording the leader -gold-tint adaptation and citing `SocialFellowshipPageController.cs:110`/`:614`. -*(Doc-only; the code behavior is a reasonable, honestly-code-documented -adaptation — the defect is purely the missing auditable row.)* +`RetailUiRuntime.cs:665-693` -### SHOULD-FIX 2 — AD-78's "35 of 50 rows dimmed" parenthetical is now stale -`38f08314`'s message claims "AD-78's derivation table gains its D7 addendum," -but the addendum landed in the CODE class-doc (in `5bdd0528`), while the -**register row itself** was not touched: AD-78's "Where" column still reads -`CharacterOptionsPageController.cs (35 of 50 rows dimmed …)` -(`retail-divergence-register.md:175`) when the post-FA4 count is **31 of 50**. -The register is "the single auditable list"; a stale count in an active row -is a bookkeeping drift. Fix: update `35`→`31` in AD-78's Where column. +```csharp +if (request.Type == (uint)GameEvents.ConfirmationType.Fellowship + && TryAutoRespondToFellowshipInvite(request)) + return true; +``` -### SHOULD-FIX 3 — D4 is not re-armed across a generation reset (reconnect) -`0x00A6` is a per-session, server-latched declaration; `_pageVisible` -(App-side UI state) is NOT reset by a generation reset, and -`ResetSessionTransientUi` (`RetailUiRuntime.cs:731-736`) does not hide/close -the social panel. So if the Fellowship tab is open across a reconnect, -`_pageVisible` stays `true`, no `OnShown`/`OnHidden`/`ActivePageChanged` -transition fires, `SetPageVisible` is never re-called, and the fresh session -never receives `0x00A6`. Since `RuntimeFellowshipState.ResetSession` clears -the roster and ACE only sends `0x02BE`/`0x02C0` while `FellowshipPanelOpen` -(gated by `0x00A6`, lane B §4.5/row 78), the roster can present empty/frozen -after reconnect until the user manually toggles the tab. This is the exact -"missed true-send freezes the roster" failure the D4 contract warns about, -just on the reconnect edge (outside the five in-session transitions, which -are all correct). Recommend re-declaring on session reset (drive -`SetPageVisible` from the reset hook, or clear `_pageVisible` so the next -`Tick`/transition re-arms) — and confirm ACE's world-enter re-send behavior -before the connected gate closes. *(I could not establish ACE's exact -world-enter `0x02BE` behavior from the client decomp; flag it for the gate.)* +**(a) No retail anchor exists.** Read in full this review: -### SHOULD-FIX 4 — no panel-level test pins the D4 conjunction -The controller-level idempotence IS tested -(`SetPageVisible_SendsPanelOpen_OnlyOnATransition`), but the panel-level -wiring in `SocialPanelController` — `OnShown`/`OnHidden`/`SwitchTo` × -active-tab → the correct `SetPanelOpen` calls, including the "close on other -tab sends nothing" and "tab-away sends false" cases — has **no** direct -assertion. The callbacks record `fellowship-set-panel-open:{bool}` -(`SocialPanelControllerTests.cs:51`) but nothing asserts on it. For the -highest-stakes mechanism, add a transition test that opens on Fellowship, -tabs away/back, and closes on each tab, asserting the exact -`set-panel-open` call sequence. +- `ClientUISystem::Handle_Character__ConfirmationRequest @0x005640A0` — + a bare 7-way jump table; `case 4` is one call to + `CM_Fellowship::SendNotice_FellowshipRequest`. No option read. +- `gmFellowshipUI::RecvNotice_FellowshipRequest @0x00490880` — copies the + string, tail-calls `MakeFellowRequestDialog`. No option read. +- `gmFellowshipUI::MakeFellowRequestDialog @0x00490620` — its **only** + guard is `if (this->m_fellowRequestContext == 0)` (`@0x00490635`), the + one-dialog-at-a-time rule lane B §2.4 already documents. No option read. +- A whole-file sweep of `IgnoreFellowshipRequests` / + `FellowshipAutoAcceptRequests` in `acclient_2013_pseudo_c.txt` returns + **only**: the two `PlayerOptionPage` label registrations + (`@0x004a0b56`/`@0x004a0d16`), an input-action toggle pair + (`@0x00561fbf`/`@0x005621ef`), `CPlayerModule::OnChanged`'s mutual + exclusion (`@0x0059a971`/`@0x0059a987`), and the `PlayerModule` + get/set switch arms (`@0x005d3aca`/`@0x005d3f11`/`@0x005d3c06`/ + `@0x005d4031`). **Zero reads on any confirmation path.** -### NIT 5 — Open/Close caption does not optimistically pre-toggle -Lane B row 77 notes retail "pre-toggles its own `_open_fellow`" on the Open -button click, i.e. the caption flips immediately client-side, then the server -confirms. acdream's `RefreshOpenCaption` keys the caption off the -server-authoritative `snapshot.IsOpen` (`:495-508`), so the caption flips -only when the `0x02BE` echo lands. Functionally convergent (and instant on -localhost), but not byte-faithful to retail's optimistic local pre-toggle. +Retail's client shows the dialog unconditionally. This is the same shape +as FA1's D9 and FA2's D2: the plan asserted a mechanism, primary source +refutes it. -### NIT 6 — `MathF.Round` vs retail `_ftol2` truncation (non-issue, documented) -`FormatStatsText` uses `(int)MathF.Round(pct*100f)` (`:650`) where retail's -`UpdateFellowStats` truncates via `_ftol2`. For every value in the even-split -table (×100 = 100/75/60/55/50/45/40/35/31.11/28) round and truncation agree, -so there is no observable difference on any reachable input. Worth a one-word -note that the choice is deliberate/equivalent; no change required. +**(b) The code comment cites ACE server source as a retail anchor.** +`RetailUiRuntime.cs:669` reads "retail's `Fellowship.cs:121`-equivalent +client-side mirror". `Fellowship.cs` is `ACE.Server`. Lane B's own rows +for these two options say the opposite of what the comment implies — +feature 5: "**Store only.** ACE is the consumer; acdream has nothing to +do beyond sending the bit (already correct)"; feature 6: "**Store +only**". + +**(c) Against a correct ACE it is dead code; against a drifting one it is +harmful.** Per lane B, ACE refuses the recruit outright when the target +has `IgnoreFellowshipRequests` (`Player_Fellowship.cs:98-102` → +`FellowshipIgnoringRequests 0x0417` to the *recruiter*) and auto-joins +without a confirmation when the target has `FellowshipAutoAcceptRequests` +(`Fellowship.cs:121` → `AddConfirmedMember(…, true)`). In both cases the +target's client never receives a type-4 request, so the intercept never +fires. It only fires when the client's copy of the bit disagrees with the +server's — and then it does the wrong thing. + +**(d) That disagreement is the default state, not an edge case.** +`CharacterOptionTable.cs:113` gives `IgnoreFellowshipRequests` a client +default of **`true`**. A player who has never touched the option, on any +server whose own default differs or that does not implement the +server-side filter, will have every fellowship invite auto-declined with +**no dialog and no chat line** — indistinguishable from a broken client. +The FA3 gate script's §FA4 steps 18/19 cannot detect this: they pass +identically whether the intercept exists or not, because ACE filters +first. + +**Recommended disposition (matching the FA1-D9 / FA2-D2 precedent):** +delete `TryAutoRespondToFellowshipInvite` and its call site, revert the +`IgnoreFellowshipRequests` / `FellowshipAutoAcceptRequests` rows in +`CharacterOptionsPageController.Groups` to `StoreOnly` (they return to +OP1's classification, which was right), and record the finding as a D6 +addendum in the plan: *the invite-receive half of D6 is refuted; both +bits are server-consumed, acdream's job is to send them, and retail's own +client shows the dialog unconditionally.* If instead the user wants the +auto-respond behavior kept as a deliberate quality-of-life divergence, +it needs (i) an explicit register row, (ii) the ACE-as-retail citation +corrected, and (iii) an answer for the default-on `Ignore` bit — but that +is a user decision, not an implementer one. --- -## Coverage confirmation -- Live-mount probe (real DATs): **PASS 1/1** — the FA4 fellowship assertions - (name field `UiField`, 11 controls `UiButton`, row template + 5 fields, - labels/captions, zero-warning `Bind()`) all hold against the production mount. -- `--no-build` targeted suites: App FA4 = 75/75, Runtime fellowship = 28/28. -- The structural finding in the ledger (Create-flow controls are children of - the not-in-fellowship frame `0x1000026B`) is confirmed by the probe dump: - `0x1000026B` children include `0x10000270/71/72/73` (all four checkboxes), - `0x1000026F` (name field), `0x10000274` (Create). +## MUST-FIX 3 — D4 never re-declares `0x00A6` after a generation reset, so a reconnect leaves the vitals stream dark for the whole new session + +`SocialFellowshipPageController.cs:453-458` + +`SocialPanelController.cs:250-268` + +`SetPageVisible` is edge-triggered on `_pageVisible`, and +`FellowshipSetPanelOpen` is a no-op while disconnected (returns +`Inactive` without sending). Enumerating every transition: + +| Transition | Behavior | Verdict | +|---|---|---| +| Panel opens with Fellowship active | `OnShown` → `_visible=true` → `SetPageVisible(true)` → sends `1` | ✓ | +| Panel opens on another tab | conjunction false, `_pageVisible` already false, no send | ✓ (retail's widget isn't visible either) | +| Switch **to** Fellowship while open | `ActivePageChanged` → sends `1` | ✓ | +| Switch **away** | sends `0` | ✓ | +| Close window while on Fellowship | `OnHidden` → sends `0` | ✓ | +| `ActivateTabs()` at mount (default = Allegiance, `Visible=false`) | conjunction false, no spurious send; ordering is safe because `ActivateTabs` runs before `RetailWindowFrame.Mount` (`RetailUiRuntime.cs:2775`) | ✓ | +| Logout | nothing sent; ACE's `LogOut_Inner` quits the fellowship anyway (lane B feature 21) and the flag is per-session server-side | ✓ — no close is owed | +| **Disconnect mid-open, then reconnect** | `_pageVisible` stays `true` across the generation reset; no visibility edge occurs on the new generation; **`0x00A6` is never sent again** | ❌ | + +`RetailUiRuntime` is process-lifetime (`GameWindow.cs:1020`, published +once behind a "already owns interaction/UI state" guard), and +`ResetSessionTransientUi` (`RetailUiRuntime.cs:731-736`) only resets the +dialog controller, the appraisal controller and the Examination window — +it does not touch the social panel. `RetailWindowLayoutPersistence`'s +`RestoreAll()` on the new `EnteredWorld` will not produce an edge either, +because `RetailWindowHandle.NotifyVisibility` early-returns when +`_notifiedVisible == visible`. + +Observable result: after any in-process reconnect with the panel left +open on Fellowship, every fellow's health/stamina/mana freezes for the +rest of the session (lane B §4.5: `Fellowship.OnVitalUpdate` sends +`0x02C0` only to fellows whose `FellowshipPanelOpen` is true, and that +flag is set only by `0x00A6`). + +**Fix:** clear the declaration latch at the existing session-reset seam +so the next `Tick`/visibility evaluation re-declares — e.g. a +`SocialPanelController.ResetSessionDeclaration()` that resets +`_fellowship`'s `_pageVisible` to `false` and then re-runs +`UpdateFellowshipPageVisibility()`, called from +`ResetSessionTransientUi`. That is a one-line addition at a seam that +already runs on every generation reset (`LiveSessionRuntimeFactory.cs:182` +→ `LiveSessionResetBindings.SessionDialogs`) — not a timer, not a poll. + +**Honesty note on the retail comparison:** `gmFellowshipUI` has no +`OnEndCharacterSession` (verified — its method list is +Create/PostInit/Update/UpdateButtons/UpdateFellow*/RecvNotice_*/ +ListenToElementMessage/OnVisibilityChanged/MakeFellowRequestDialog and +nothing else), and its `PostInit` does **not** send `Event_UpdateRequest` +(lane A §6.3 — only the allegiance panel does). Retail's re-declaration +therefore comes from `OnVisibilityChanged` firing when the in-game UI +state is torn down/restored around character select; that inference is +not byte-established here. The *defect* stands regardless of how retail +gets it right: acdream reaches a state where the stream it just turned on +is off and nothing will ever turn it back on. + +**Also:** the gate script §FA4 has no reconnect step, so the owed +connected gate cannot catch this. Add one after step 8. + +--- + +## MUST-FIX 4 — `gmFellowshipUI::UpdateFellowSelection` is not ported: selecting a fellow in the WORLD leaves Dismiss/Leader disabled, and no row ever shows as selected + +The slice ports one direction of retail's two-directional selection +coupling. `SelectFellow` (`SocialFellowshipPageController.cs:676-680`) +correctly does panel → world (`SelectionChangeSource.Social`, the FA4 +Runtime commit). The reverse arm is missing: + +`gmFellowshipUI::UpdateFellowSelection @0x0048F0F0`, reached from +`RecvNotice_SelectionChanged @0x0048F1C0` (and from `Update` +`@0x0048F6E9`), walks the list box on every world-selection change: + +``` +selectedID = ACCWeenieObject::selectedID +for each row: + if row.GetAttribute_InstanceID(0x1000000D) == selectedID: + m_iidSelectedFellow = selectedID + SetSelectedItem(listBox, row, 0) // and return + else if that id == m_iidSelectedFellow: + remember row as the fallback +SetSelectedItem(listBox, fallbackOrNull, 0) +UpdateButtons(this) +``` + +So in retail, clicking a fellow **in the 3D world** (or reaching them via +any other selection origin) selects their panel row and enables +Dismiss/Assign-Leader. In acdream `_selectedFellowGuid` is written only +by the row name-text `OnClick` +(`SocialFellowshipPageController.cs:599,676`), so the world→panel arm +does nothing and those two buttons stay greyed. + +Two further consequences of the same gap: + +- **No visual selection at all.** There is no `SetSelectedItem` + equivalent — nothing in the roster ever indicates which fellow is + selected. The user clicks a name and gets no feedback until they notice + two buttons un-greying. +- **The plan's FA4 contract line is not met.** §3's FA4 row reads + "roster rows (adds `UiTemplateListBox` Flush/selection/row-instance-id + — lane A sized this)". `Flush` pre-dated FA4 (FA3); this slice added + `FlushPreservingScroll` only. **No selection model and no per-row + instance-id were added** — retail's row identity key is + `SetAttribute_InstanceID(row, 0x1000000D, fellowIid)` (lane A §3.1 row + table, `ghidra@0x0048F440:95`), the exact primitive both + `ListenToElementMessage` and `UpdateFellowSelection` read. The ledger's + four-item "Contradictions/deferrals" list does not mention this. + +Minimum fix: give `UiTemplateListBox` a row-instance-id + selected-row +concept (or, if that is deferred, say so explicitly in the ledger and +file the register row), and subscribe the controller to +`SelectionState.Changed` — or simply re-derive `_selectedFellowGuid` from +`Selection.SelectedObjectId` in `RefreshButtonStates` when the selected +world object is a member — so the world→panel arm exists. + +--- + +## MUST-FIX 5 — three shipped deviations have no register row (CLAUDE.md's register rule, plan §5) + +The register rule is binding and explicit: *"Any commit that introduces a +deviation adds its register row IN THE SAME COMMIT… A deviation found +without a row is a bug twice over."* `38f08314` files AD-80 (D5 display +vs ACE) and AD-81 (StringInfo/FormatName) and amends AD-78. Missing: + +1. **The D6 client-side invite intercept** — a client behavior retail + does not have (MUST-FIX 2). If it survives review at all it needs its + own row; if it is deleted, the row is not owed. +2. **The gold leader-name tint** (`LeaderNameColor` `(1, 0.84, 0, 1)`, + `SocialFellowshipPageController.cs:110,614`). This is an invented + user-visible color with no DAT or decomp anchor — the class doc says + so in as many words ("a minimal, clearly-adaptive visual cue rather + than inventing a DAT mechanism that was not found"). That is precisely + an AD row: a reviewer comparing a retail screenshot will see a color + retail never paints. Same class as AD-78's own rationale. +3. **Row selection restricted to the name text.** Retail's list box + selects on the *row* (`ListenToElementMessage` message `3`/`0x42`, + lane B §2.8). acdream binds `OnClick` to the row's name `UiText` only, + so clicking the stats text, any meter, or row whitespace does nothing. + Documented in `SelectFellow`'s doc comment; not registered. + +Separately, the ledger's decision to keep the **Recruit "is the target a +player" gate** as "an inline comment, not a register row" is the wrong +call by the same rule. Retail *disables* the button +(`UpdateButtons`, lane B §2.8: "selected world object **is a player**, +not already a fellow, and `!IsFull`"); acdream enables it and lets the +server refuse. That is a divergence with a user-visible symptom (a lit +Recruit button that does nothing when a chest is selected), and the +justifying comment — "exactly like retail's own silent no-op" — is +inexact, because retail's click handler is unreachable when the button is +disabled. + +--- + +## SHOULD-FIX + +**SF-1 — `RefreshFellowshipName` reintroduces the per-frame `LinesProvider` +closure allocation that FA3's fix round removed from the sibling page one +commit earlier.** +`SocialFellowshipPageController.cs:489-493` assigns +`() => [new UiText.Line(name, MemberNameColor)]` on **every** `Tick` +while in a fellowship — a display-class + delegate allocation per frame, +and `SocialPanelController.Tick` calls the fellowship page +unconditionally, so it allocates while the panel is hidden too. +`35c40a9b` fixed exactly this in `SocialAllegiancePageController` +(hoisted `BlankLineProvider`/`NoLinesProvider` statics, doc: "zero +allocation while idle") in response to FA3 mechanism SF-2 / blast SF-4. +Cache the provider and only reassign when `name` actually changes. + +**SF-2 — the commit message's carry-forward-4 claim does not describe the +code.** The message says "The Fellowship roster path never advances its +revision latch on a partial resolver failure until the NEXT real +membership change — never a per-frame retry loop." +`SocialFellowshipPageController.cs:433-437` advances `_lastRosterRevision` +**unconditionally**, before `RefreshRoster` runs. What actually happens +on a permanently-unbuildable template: `RebuildRoster` `continue`s past +the failed row (`:580-586`), leaving `_rows.Count < members.Count`, so +`membershipChanged` is true on **every** subsequent revision bump — i.e. +a full `FlushPreservingScroll` + N `LayoutImporter.Build` calls under the +shared DAT lock on every incoming `0x02C0` vitals tick, not "until the +next real membership change". `RowTemplateResolver` bounds the *import* +(a null import is cached, `RowTemplateResolverTests` pins it) but not the +*build*. Either make the claim accurate or bound the rebuild (e.g. latch +a per-generation "row template is unbuildable" flag). + +**SF-3 — Recruit's "already a fellow" test reads render rows, not +membership.** `SocialFellowshipPageController.cs:527` uses +`!_rows.ContainsKey(id)`. Retail uses `Fellowship::IsFellow`. If a row +failed to build (SF-2's path), an existing fellow passes `targetValid` +and Recruit lights up for someone already in the fellowship. Use the +member set. + +**SF-4 — `SocialPanelController.Dispose` does not unsubscribe +`ActivePageChanged`.** `:140` subscribes a `this`-capturing lambda; +`:305-309` only sets `_disposed`. `Tick()` guards on `_disposed` +(`:295`); the event handler does not, so a tab switch after disposal +still reaches `SetPageVisible` → a Runtime command. Add the unsubscribe +(or a `_disposed` check in `UpdateFellowshipPageVisibility`). + +**SF-5 — test naming/coverage gaps on the slice's headline mechanisms.** +- `Tick_MembershipChange_RebuildsRoster_ButPreservesScrollPosition` + (`SocialFellowshipPageControllerTests.cs:244`) asserts only + `Children.Count == 2`; its own comment admits it cannot observe the + preserved offset. Rename it or build a roster tall enough to scroll. + (The widget-level `UiTemplateListBoxFlushPreservingScrollTests` **do** + cover the shrink case correctly — re-derived: `ClearContent` zeroes + `UiScrollablePanel.ContentHeight` but not `UiScrollable.ContentHeight`, + so the restore clamps against the stale height, and + `LayoutScrollableChildren` re-clamps against the fresh height at the + head of the next `OnDraw`, before anything paints. That is sound.) +- **D4's actual conjunction is untested.** The only D4 test is + `SetPageVisible_SendsPanelOpen_OnlyOnATransition` on the *page* + controller. Nothing tests `SocialPanelController`'s + `_visible && IsShowingFellowship` logic, the `ActivePageChanged` + subscription, or `OnShown`/`OnHidden` — the mechanism the slice is + named for. `SocialPanelControllerTests` gained a bindings helper and + zero new tests. +- No controller-level test for a member **leaving** (the shrink rebuild), + and none for `_selectedFellowGuid` being cleared when the selected + fellow departs (`:568-569`). + +**SF-6 — the live-mount probe prints the strings it is cited for but does +not assert them.** `SocialPanelLiveMountProbeTests.cs:185-201` writes the +four checkbox labels and the two captions to the console with no +assertion, yet the ledger's live-DAT paragraph cites them as verified. +This is the same finding FA3's own mechanism SF-3 raised ("printed but +never asserted — deserves a real assertion, not just a hope") for +`0x10000492` and page exclusivity. Assert non-null (and, for the two +captions, the exact `"Open"`/`"Close"`). + +**SF-7 — gate-script §FA4 defects (the FA3 MF-1/MF-2 class).** +- Step 3 contradicts itself: "Only the Quit button should be enabled … + Disband and Open should ALSO be enabled." A tester following the first + clause reports correct behavior as a defect. +- Steps 18/19 claim to gate D6, but per lane B ACE filters both bits + server-side and never sends the confirmation, so both steps pass + identically with or without the client intercept. They cannot fail. +- No reconnect step (MUST-FIX 3) and no six/eight-fellow percentage step + (MUST-FIX 1) — the two defects most likely to reach a user are both + outside the script's reach. + +**SF-8 — D7's `FellowshipShareLoot` un-dim does not meet AD-78's own +definition of "Live".** AD-78 defines the dimmed set as rows that +"persist and, where auto-save, send the wire bit, but drive nothing +observable client-side". `FellowshipShareLoot`'s stated new consumer is +"a SECOND live checkbox surface on the fellowship page itself" — but a +second editor of the same stored value is not a consumer, nothing in +acdream reads the bit (`FormatStatsText` uses `snapshot.ShareXp` only, +and the `0x00A2` Create builder carries `shareXP` alone), and the live +DAT dump confirms that checkbox (`0x10000273`) is a child of the +NOT-in-fellowship frame `0x1000026B`, i.e. **invisible whenever you are +in a fellowship**. Re-dim it, or state the widened definition explicitly +in AD-78. (`FellowshipShareXP` is defensible: the Create click genuinely +reads it, `:325-326`.) The same "second surface" argument in +`CharacterOptionsPageController`'s doc should be corrected either way. + +--- + +## NIT + +**N-1 — meter text vs authored child.** Retail's `UpdateFellowVitals +@0x0048ED60` sets meter attribute `0x69` to `cur/max` and writes the +cur/max ints into the meter's authored **child** text elements +(`0x10000286` under health, `0x10000288` under stamina). acdream uses +`UiMeter.Fill`/`UiMeter.Label` because `UiMeter.ConsumesDatChildren` is +true — a pre-existing widget decision, not FA4's, but worth one line in +`SetVitals`'s doc so the next reader does not go hunting for the +unbound `0x10000286`/`0x10000288`/`0x1000028A`. + +**N-2 — `max > 0` guard.** `SetVitals` (`:661`) returns `0f` when +`max == 0`; retail divides unconditionally. The guard is the right +engineering call; just say so, since "verbatim port" is claimed nearby. + +**N-3 — snapshot/roster read atomicity.** `Tick` takes +`_bindings.Snapshot()` and `_bindings.Members()` under two separate +acquisitions of `RuntimeFellowshipState._gate`, so `Revision` / +`MemberCount` can momentarily disagree with the returned roster. It is +self-correcting on the next revision bump and harmless in practice +(single-threaded tick today), but a one-line comment would stop a future +reader from assuming atomicity. + +--- + +## What is faithful (checked, no action) + +- **Button-enable table** (`RefreshButtonStates`, `:510-537`) matches lane + B §2.8 exactly, including the non-obvious Recruit rule + (`isLeader || snapshot.IsOpen`) and Quit-always-enabled. +- **Open/Close caption reads as the ACTION, not the state** (`:495-508`), + resolved once at `Bind` and swapped from cache — correct, and correctly + refuses to invent English when the strings do not resolve. +- **Create gating** — empty/whitespace name disables the button and the + click double-checks (`:322-327`, `:460-465`); no invented error text, + matching lane B §2.2's "the button IS the refusal mechanism". +- **Quit/Disband share `0x00A3` with the flag flipped**, and both route + through `IRuntimeFellowshipCommands.Quit` so the retail pre-quit + `0x0290` leader hand-off in `RequiresLeaderHandoffBeforeQuit` is not + bypassed — the reason the commit gives for not calling `WorldSession` + directly is correct and load-bearing. +- **The roster diff never rebuilds on a vitals tick** — the FA3 + carry-forward's exact hazard. Verified by construction (set comparison + at `:551-560`, in-place `UpdateRow` otherwise) and by + `Tick_SameMemberSet_UpdatesRowsInPlace_NoRebuild`'s `Assert.Same` on + the row instance. +- **`FlushPreservingScroll` shrink semantics** — re-derived against + `UiScrollablePanel`/`UiScrollable`; correct, and pinned by + `FlushPreservingScroll_ClampsToTheNewShorterContent`. +- **`EvenSplitPercentTable` and the `>= 9` full check** — byte-verified + against the binary, including the out-of-range `0.0` default matching + retail's unsigned `ja` fallthrough for n=0. +- **`RowTemplateResolver`** — clean extraction, caches the import (miss + included) and rebuilds per row; carry-forward 2 satisfied. +- **`IRuntimeFellowshipView.GetMembers`** — materialized under the same + lock as every other read, honest ordering caveat in its doc. +- **`SelectionChangeSource.Social`** — additive; no exhaustive switch on + the enum exists in production, so no dispatch site was missed.