acdream/docs/research/2026-08-12-fa4-review-mechanism.md
Erik 06dbf1cf8f docs: FA4 MF-3 REOPEN re-fix re-review -- CLOSED (04161def)
The re-fix moves the 0x00A6 re-declaration off the pre-world reset seam
and onto the post-world EnteredWorld seam, and stops the widget latch from
advancing on a dropped publish. Verified in the diff:

- SetPageVisible advances _pageVisible ONLY on RuntimeCommandStatus.Accepted
  (the widget-level root of the REOPEN); a dropped Inactive publish leaves
  the latch clear so the in-world attempt is not deduplicated.
- ResetSessionDeclaration (pre-world) now only clears the latch;
  RedeclareAfterWorldEntry (new) does the re-evaluation, wired through
  RetailUiRuntime.RedeclareSocialPanelAfterWorldEntry into
  LiveSessionRuntimeFactory's EnteredWorld RestoreLayout delegate.

Seam ordering traced and confirmed inverse of the pre-world SessionDialogs
stage: StartCore runs ResetHostBeforeStart (pre-world reset, latch clear)
at :555, then ActivateCommands :639, _inWorld=true :642, and
ApplyEnteredWorld :644 -> LiveSessionHost.ApplyEnteredWorld ->
RestoreLayout delegate -> RedeclareAfterWorldEntry. So SetPanelOpen's
requireWorld gate is Accepted and 0x00A6 publishes on the fresh server.
Idempotent and load-bearing (the social panel isn't state-managed
visibility, so RestoreLayout fires no OnShown edge).

Tests model the world gate (fake returns Accepted only when in-world) and
would fail against pre-fix behavior: the widget test's second attempt is
deduplicated if the latch advances unconditionally; the reconnect test's
DoesNotContain-after-reset fails if the pre-world declaration is
reintroduced (the coordinator's RED-verification). Binary confirmed
post-fix (new tests reference RedeclareAfterWorldEntry); 3/3 new + 58/58
touched classes green. 13,286/4/0 reconciles (+1, 0 deletions).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-08-12 08:02:59 +02:00

47 KiB
Raw Permalink Blame History

Campaign FA slice FA4 — MECHANISM-FAITHFULNESS review

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, 9 SHOULD-FIX, 4 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.


0. What this review re-derived (not trusted)

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 EXTENDED0x1000026B 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

MUST-FIX 1 — D5's percentage conversion rounds where retail truncates: 6- and 8-fellow rows display the wrong number

SocialFellowshipPageController.cs:650

return $"{member.Level}  {(int)MathF.Round(pct * 100f)}%";

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.

Retail, byte-decoded from the PDB-paired binary at gmFellowshipUI::UpdateFellowStats:

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

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.

Per-entry comparison (float bits read from the binary; acdream column is the shipped expression):

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

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:

return $"{member.Level}  {(int)((double)pct * 100.0)}%";

((double)0.44999998807907104 * 100.0 = 44.999998807907104 → 44 ✓; 0.3499999940395355 * 100.0 = 34.99999940395355 → 34 ✓; every other row unchanged.)

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.

MUST-FIX 2 — D6's invite intercept is a client-side mechanism retail does not have, and it can silently swallow real invites

RetailUiRuntime.cs:665-693

if (request.Type == (uint)GameEvents.ConfirmationType.Fellowship
    && TryAutoRespondToFellowshipInvite(request))
    return true;

(a) No retail anchor exists. Read in full this review:

  • 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.

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.

(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-102FellowshipIgnoringRequests 0x0417 to the recruiter) and auto-joins without a confirmation when the target has FellowshipAutoAcceptRequests (Fellowship.cs:121AddConfirmedMember(…, 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.


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=trueSetPageVisible(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:182LiveSessionResetBindings.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 continues 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.

SF-9 — AD-78's own row still says "35 of 50 rows dimmed". (Carried from the superseded review, verified independently.) 38f08314's message claims "AD-78's derivation table gains its D7 addendum", but the addendum landed in CharacterOptionsPageController's class doc (in 5bdd0528); the register row's own "Where" column still reads CharacterOptionsPageController.cs (35 of 50 rows dimmed …) while the post-FA4 count is 31 of 50. The register is "the single auditable list" — a stale count in an active row is exactly the drift AD-78's own "Risk" column warns about. (Note this interacts with SHOULD-FIX 8 and MUST-FIX 2: if either lands, the correct number is not 31 either. Fix the count last.)


NIT

N-0 — the Open/Close caption does not optimistically pre-toggle. (Carried from the superseded review, verified.) Lane B feature 11 records that retail's Open button handler "client pre-toggles its own _open_fellow" before Event_ChangeFellowOpeness — i.e. the caption flips immediately and the server echo confirms it. RefreshOpenCaption (:495-508) keys purely off server-authoritative snapshot.IsOpen, so the caption flips only when the 0x02BE echo lands. Convergent, and invisible on localhost; not byte-faithful.

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.

Appendix — collision with a concurrent review at 6849b457

A second mechanism-lens pass landed on this same path at 6849b457 (2026-08-12 04:59:35) while this one was in progress; this document replaces it. Its text is not lost — read it with git show 6849b457:docs/research/2026-08-12-fa4-review-mechanism.md. Its two unique findings are carried forward above (SF-9, N-0). The two reviews disagree in exactly two places, both adjudicated here from primary source:

Topic 6849b457 This review Why
MathF.Round vs _ftol2 NIT 6, "non-issue" — "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" MUST-FIX 1 That reasoning uses the DECIMAL literals. The stored constants are floats: 0x007C91D4 = 0.44999998807907104 and 0x007E72BC = 0.3499999940395355 (byte-read from the PDB-paired binary this review), so retail's ×100 products are 44.999998… and 34.999999… and _ftol2 chops them to 44 and 34. Two of ten roster sizes differ. This is precisely the campaign's §6 rule ("BN literal-0 operands are byte-verified before use") applied one level deeper — to the .rdata float, not the decompiler's rendering of it
D6 invite auto-response "✓ verified clean" — describes the intercept's shape and Runtime's mutual exclusion, then passes it MUST-FIX 2 The review verified that the code does what the PLAN says. Mechanism faithfulness asks whether RETAIL does it. Handle_Character__ConfirmationRequest @0x005640A0, RecvNotice_FellowshipRequest @0x00490880 and MakeFellowRequestDialog @0x00490620 were read in full here: no option read on any confirmation path, and a whole-file sweep of both accessors finds reads only in chargen, the input-action toggle, OnChanged's mutual exclusion, and the PlayerModule get/set switch

The two reviews AGREE on D4-not-re-armed-across-reconnect (its SHOULD-FIX 3, raised here to MUST-FIX 3 because it silently disables the stream the slice exists to enable), on the missing panel-level D4 conjunction test (its SHOULD-FIX 4 = SF-5 here), on the leader-gold-tint register omission (its MUST-FIX 1 = part of MUST-FIX 5 here), and on the live-DAT probe result.


Narrow re-review of the fix round — 2026-08-12

Fix commits: 290f9b58 / 5499f058 / df000306 / 300d8189 / 55b17e15 / 1d743277 / f041b09b.

Verdict: CLOSED with ONE REOPEN — MUST-FIX 3. Dispositions re-derived from the actual diffs (not the commit claims), the D6/D7/SF-8 dimming arithmetic audited from primary source, totals reconciled on the touched projects, and the live-mount probe re-run on the post-fix binaries. Four of five MUST-FIX, all 9 SHOULD-FIX, all 4 NIT, and blast SF-1 are correctly applied. MUST-FIX 3 is REOPENED: the latch-clear it ships is necessary but its re-declaration never reaches the wire in the real reconnect path, so the fellow-vitals-freeze defect it targets persists.

MUST-FIX 1 — CLOSED

FormatStatsText now returns $"…{(int)((double)pct * 100.0)}%" (SocialFellowshipPageController.cs:790). The two failing sizes are pinned: [InlineData(true, true, 6, "12 44%")] and [InlineData(true, true, 8, "12 34%")] added to FormatStatsText_MatchesD5Rules. Gate-script step 23 corrected to "the panel must show 44% and 34% respectively — NOT 45% or 35%". AD-80's "renders retail's byte-decoded table verbatim" claim is now true of the rendered integer, not just the table — no further AD-80 edit owed.

MUST-FIX 2 — CLOSED

TryAutoRespondToFellowshipInvite deleted; HandleConfirmationRequest is now => _gameplayConfirmationController?.HandleRequest(request) == true (RetailUiRuntime.cs:678), routing every type through the generic controller exactly as retail Handle_Character__ConfirmationRequest @0x005640A0 does. New test FellowshipInviteRequest_Type4_OpensDialog_MessageVerbatim_AndSendsAcceptOnClose proves the type-4 dialog renders the server message verbatim (not " Continue?"-suffixed) and sends accept through the generic path. No allegiance (type-1) path broke: the deleted intercept keyed only on ConfirmationType.Fellowship (type 4) — type 1 always fell through to the generic controller both before and after the fix (verified against the pre-fix 5bdd0528 source). The IgnoreFellowshipRequests / FellowshipAutoAcceptRequests rows revert with the dimming reversal below.

MUST-FIX 3 — REOPEN (latch-clear correct, re-declaration swallowed)

The fix adds SocialPanelController.ResetSessionDeclaration() (clears the edge-latch via SocialFellowshipPageController.ResetPageVisibleLatch(), then re-evaluates the D4 conjunction) and wires it into RetailUiRuntime.ResetSessionTransientUi(). The direction is right; the timing is wrong, and the unit test cannot see it because its fake command records unconditionally.

Traced from source:

  • ResetSessionTransientUi runs only via the SessionDialogs reset stage (LiveSessionRuntimeFactory.cs:182 -> LiveSessionResetManifest).
  • Both ResetSessionState call sites run that stage before the new session is in-world: ResetHostBeforeStart (LiveSessionController.cs:733-751, run inside StartCore before _inWorld = true at :642 and before binding.ActivateCommands()), and the retired-scope teardown (:231, which tears down the OLD scope).
  • SetPanelOpen requires world: Validate(expectedGeneration, requireWorld: true) (CurrentGameRuntimeCommandAdapter.cs:929), and the deferred App seam is not even bound to the new generation until ActivateCommands. So during the reset the send returns Inactive/rejected and nothing is published.
  • But ResetSessionDeclaration -> SetPageVisible(true) still sets _pageVisible = true (the transition fires the no-op command). No post-world-entry hook re-evaluates the conjunction (the EnteredWorld bindings — RestoreLayout/SyncToolbar/etc. — never touch the social panel's visibility, and the panel is not a state-managed-visibility window so layout restore produces no OnShown edge; Tick never calls SetPageVisible).

Net: after an in-process reconnect with the Fellowship page left open, _pageVisible ends true but 0x00A6 was never sent to the new server — the exact vitals-freeze this MUST-FIX targets (lane B §4.5) persists. The fix relocated the latch reset without achieving the observable outcome. The gate script's new reconnect step (step 9) will catch this at the owed connected gate — so this is not silently hidden — but a green automated gate here is not evidence the wire send happens, and the fixer's "resolved" claim is not substantiated in the real path.

Recommended direction: re-declare while in-world on the new session, not during the pre-world reset — e.g. re-run the D4 conjunction from the EnteredWorld seam (LiveSessionRuntimeFactory.cs:158-164) after clearing the latch, or make SetPageVisible refuse to advance _pageVisible when the command result is not Accepted (paired with an in-world evaluation trigger). Either is a one-seam change; the current placement cannot work because the reset is a pre-connect, local-state-clearing phase.

MUST-FIX 4 — CLOSED (deferral is minimal-observable-contract, audited)

SyncSelectionFromWorld/SetSelectedFellow reproduce retail UpdateFellowSelection @0x0048F0F0: the "found" arm (a world selection matching a member becomes the panel selection, enabling Dismiss/Leader and tinting the row) and the "fallback" arm (a non-member world selection leaves the existing panel selection untouched; RefreshRoster clears it only when that fellow leaves the roster). Called every Tick while in a fellowship, mirroring retail's per-Update placement. The generic UiTemplateListBox selection-model + 0x1000000D row-instance-id port is not done — but AD-82(4) records that honestly as owed, and the OBSERVABLE contract (button-enable + a highlight) is met. This is a scoped, declared page-local reimplementation, not a hidden gap: the only observable divergences (a blue selection tint retail never paints; only the name text is a click target) are both named in AD-82.

MUST-FIX 5 — CLOSED

AD-82 (invented leader/selection tints, name-text-only click target, page-local world->panel sync with the generic port recorded as owed) and AD-83 (Recruit "is-a-player" enable-gate omission) are well-formed: correct file/symbol cites, honest "Risk" columns, real decomp anchors (UpdateFellowSelection @0x0048F0F0, ListenToElementMessage @0x004901C0, UpdateButtons §2.8). The prior "inline comment, not a register row" call on the Recruit gate is corrected. AD-78's stale "35 of 50" is fixed to "34 of 50 / 16 live" and self-flags the two-campaign register lag as an instance of the exact risk that row warns about.

D6/D7/SF-8 dimming arithmetic — CLOSED and CORRECT (audited from source)

Landed at 34 of 50 dimmed / 16 live — one net un-dim from the pre-FA4 baseline (FellowshipShareXP only), not "splitting the difference". Verified from primary source, not the commit message:

  • FellowshipShareLoot -> dimmed is faithful. Grep of src/ finds exactly two live references (SocialFellowshipPageController.cs:437 binds the page checkbox; :578 seeds it from the value) — both an editor/display surface, neither a reader of the value. The 0x00A2 Create builder carries shareXP alone (SocialActions.cs:139-141); nothing in acdream reads FellowshipShareLoot to drive a wire message or client behavior (ACE authors loot-sharing and its chat lines server-side). A second editor of a stored value is not a consumer — this is precisely AD-78's own store-only definition.
  • FellowshipShareXP -> Live is right. The Create-button OnClick (SocialFellowshipPageController.cs:377) reads CurrentCharacterOption(FellowshipShareXP) and passes it as the sent shareXP bit — a genuine client-side read of the value that changes what the wire carries (lane B feature 1: "shareXP comes from option 0x0F"). That is the exact asymmetry that separates it from ShareLoot.
  • IgnoreFellowshipRequests/FellowshipAutoAcceptRequests -> dimmed: their only claimed consumer was the deleted intercept; both are pure server-side filters. Correct. Conformance test updated to Assert.Equal(34, …) / Assert.Equal(16, …) with the three ids added back to ExpectedStoreOnlyIds.

SHOULD-FIX / NIT — all applied (spot-verified)

SF-1 (_fellowshipNameLinesProvider cached, reassigned only on a real name change — :583); SF-2/SF-3 (_memberGuids is the source of truth for the membership diff and Recruit's already-a-fellow test, :626/:657 — no longer _rows.Count); SF-4 (Dispose unsubscribes the stored _onActivePageChanged); SF-5 (the D4 panel-level conjunction is now tested by FellowshipPageVisible_Declares0x00A6_OnlyWhenWindowShownANDFellowshipActive, plus a member-leaves shrink test and a selection-clear test); SF-6 (the probe now Asserts the four labels non-empty and the captions exactly "Open"/"Close"); SF-7 (gate step 3 self-contradiction reconciled); SF-8 (see dimming above); SF-9 / N-0 / N-1 / N-2 / N-3 (AD-78 count, optimistic caption pre-toggle, meter-child + max>0 + snapshot-atomicity doc notes) all present.

Totals and probe

ACDREAM_PROBE_LIVE_MOUNT=1 re-run on the post-fix binaries: PASS 1/1 — the four checkbox labels resolve, captions resolve Open/Close, tab captions Allegiance/Fellowship/Friends/Squelch. The six touched App test classes pass 109/109 on the post-fix Release binaries (rebuilt 07:36). The ledger's +13 / 0-deletion delta reconciles by direct count (8 in SocialFellowshipPageControllerTests = 2 new [InlineData] + 6 [Fact]; 4 [Fact] in SocialPanelControllerTests; 1 [Fact] in GameplayConfirmationControllerTests; the two count-only changes are net-0). The full-suite 13,285/4/0 claim is corroborated on the touched projects and by arithmetic (13,272 + 13); it was not re-run end-to-end here.

Disposition

One REOPEN (MUST-FIX 3 — re-declaration placed at a pre-world reset seam, so 0x00A6 never reaches the reconnected server). Everything else CLOSED, with the dimming arithmetic audited correct at 34/16. Recommend a focused MUST-FIX-3-only re-fix (move the re-declaration to an in-world seam); no re-review of the other findings is owed.


MF-3 REOPEN re-fix re-review — 2026-08-12 (commit 04161def)

Verdict: CLOSED. The seam ordering the whole fix rests on holds — the EnteredWorld re-declaration is genuinely post-world, the inverse of the pre-world ResetSessionState/SessionDialogs stage the original fix used. The widget-level root I named is addressed, the tests model the world gate (not an unconditional fake), and the full suite reconciles.

The two halves, verified in the diff

  1. Widget-level (the root I named). SocialFellowshipPageController.SetPageVisible now advances the latch ONLY on Accepted: if (_bindings.SetPanelOpen(visible).Status == RuntimeCommandStatus.Accepted) _pageVisible = visible; A dropped/Inactive publish (the pre-world reconnect state) leaves _pageVisible untouched, so the in-world attempt is not deduplicated away by the if (_pageVisible == visible) return guard. The normal in-world case is unchanged (Accepted → latch advances as before), and a persistently non-Accepted result retries at most once per generation (EnteredWorld fires once per generation) — no retry loop.
  2. Lifecycle split. ResetSessionDeclaration (pre-world) now ONLY clears the latch (=> _fellowship?.ResetPageVisibleLatch();) — it no longer calls UpdateFellowshipPageVisibility, so it makes no dropped pre-world declaration. The new RedeclareAfterWorldEntry() (=> UpdateFellowshipPageVisibility();) does the re-evaluation, exposed as RetailUiRuntime.RedeclareSocialPanelAfterWorldEntry and composed into LiveSessionRuntimeFactory's EnteredWorld.RestoreLayout delegate (RestoreLayout first, then redeclare).

Seam ordering — traced, and it holds (the load-bearing claim)

The fix is correct only if EnteredWorld runs after _inWorld = true and after the command seam is active. It does. In LiveSessionController.StartCore:

  • ResetHostBeforeStart (:555) → host.ResetSessionStateLiveSessionLifecycleHost.ResetSessionState (:47) → _bindings.Reset → the reset manifest's SessionDialogs stage → ResetSessionTransientUiResetSessionDeclaration (clears the latch). PRE-world_inWorld is still false here.
  • binding.ActivateCommands() (:639) — the deferred command seam binds for the new generation.
  • _inWorld = true (:642).
  • host.ApplyEnteredWorld(selection) (:644) → LiveSessionLifecycleHost.ApplyEnteredWorld (:57) → _bindings.EnteredLiveSessionHost.ApplyEnteredWorld (:212) → _enteredWorld.RestoreLayout() (:216) → the re-fixed delegate → RedeclareSocialPanelAfterWorldEntryRedeclareAfterWorldEntryUpdateFellowshipPageVisibilitySetPageVisible(true). POST-world_inWorld is true and commands are active, so SetPanelOpen's Validate(requireWorld: true) passes → Accepted → 0x00A6 published → latch advances. Fellow vitals resume on the fresh server.

This is the exact inverse of the pre-world SessionDialogs stage traced in the REOPEN, and it is idempotent: the social panel is not a state-managed-visibility window, so RestoreLayout does not re-show it and no OnShown edge fires — RedeclareAfterWorldEntry is therefore load-bearing (not redundant) in the reconnect-with-panel-open case, while still a safe no-op if some other path had already re-declared (latch already set).

Tests genuinely exercise the world gate (and would fail against pre-fix)

The fake SetPanelOpen now returns AcceptedResult when its inWorld/PanelOpenInWorld flag is true and InactiveResult when false — a real world-gate model, not an unconditional recorder.

  • SetPageVisible_DoesNotLatch_WhenDeclarationDropped_SoItRetriesInWorld: sets the flag false, SetPageVisible(true) records the dropped attempt; flips the flag true, SetPageVisible(true) AGAIN records a send. Against the pre-fix unconditional _pageVisible = visible, the second call would early-return (deduplicated) and record nothing — the Assert.Contains would fail. So this pins the widget-level root.
  • Reconnect_ReDeclares0x00A6_AfterWorldEntry_NotDuringPreWorldReset: after the in-world declare, sets in-world false, calls ResetSessionDeclaration() and asserts DoesNotContain(...set-panel-open...) — this is exactly the assertion that fails if the pre-world declaration is reintroduced (matching the coordinator's RED-verification); then sets in-world true, calls RedeclareAfterWorldEntry() and asserts the send lands.
  • Reconnect_StaysSilent_WhenFellowshipPageIsNotActuallyOpen: default tab Allegiance; after reset + post-world redeclare, the D4 conjunction is false, so no send — the counterpart holds.

Build currency and totals

The current App test binary is post-04161def: the three new tests reference SocialPanelController.RedeclareAfterWorldEntry, a method that exists only in the fixed source — the project would not compile against pre-fix source, so a resolving+passing run proves the binary reflects the fix. Ran on it: the three new tests 3/3, and the three touched classes (SocialFellowshipPageControllerTests / SocialPanelControllerTests / GameplayConfirmationControllerTests) 58/58. The 13,286/4/0 full-suite claim reconciles by direct count: net +1 test from the prior 13,285 (the one new widget [Fact]; the two panel tests were renamed in place, net 0), 0 deletions. Not re-run end-to-end here.

Disposition

MF-3 is CLOSED. All five MUST-FIX, all 9 SHOULD-FIX, all 4 NIT, and blast SF-1 are now correctly applied; the D6/D7/SF-8 dimming stands at the audited-correct 34/16. The FA4 fix round is fully resolved on the mechanism lens; only the user's connected gate (several steps [TWO-CLIENT], deferrable to FA6) remains owed.