docs: FA4 mechanism review -- APPROVE-WITH-FIXES (1 MUST-FIX, 3 SHOULD-FIX, 2 NIT)
Mechanism-faithfulness lens on 357d2032/5bdd0528/38f08314. Every wire-touching mechanism verifies retail-faithful: the D4 0x00A6 gate (all five in-session transitions + idempotence + no-send-while- disconnected), the leader-quit 0x0290-before-0x00A3 hand-off routing, the D5 byte-exact even-split table, the D6 type-4 auto-response + Runtime mutual exclusion, the D7 four-row un-dim (35->31 conformance), scroll preservation across a rebuild, create-flow refusal-by-enabled- state, and the button-enable rules. Live-mount probe PASSES 1/1 against real DATs; FA4 suites 75/75 App + 28/28 Runtime under --no-build. MUST-FIX: the leader-gold-tint adaptation has no divergence-register row (AD-80/AD-81 don't cover it). SHOULD-FIX: AD-78's stale 35-of-50 count; D4 not re-armed across a reconnect while the panel stays open; no panel-level test pins the D4 conjunction. NITs: caption pre-toggle, _ftol2-vs-Round (both non-blocking). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
parent
eda8729c92
commit
6849b45771
1 changed files with 263 additions and 0 deletions
263
docs/research/2026-08-12-fa4-review-mechanism.md
Normal file
263
docs/research/2026-08-12-fa4-review-mechanism.md
Normal file
|
|
@ -0,0 +1,263 @@
|
|||
# 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.
|
||||
|
||||
---
|
||||
|
||||
## VERDICT: APPROVE-WITH-FIXES
|
||||
|
||||
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.
|
||||
|
||||
---
|
||||
|
||||
## What verified CLEAN (re-derived, not trusted)
|
||||
|
||||
### 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;`).
|
||||
|
||||
- **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. ✓
|
||||
|
||||
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. ✓
|
||||
|
||||
**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`).
|
||||
|
||||
### 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`). ✓
|
||||
|
||||
### 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. ✓
|
||||
|
||||
### 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. ✓
|
||||
|
||||
### 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`). ✓
|
||||
|
||||
### 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`). ✓
|
||||
|
||||
### 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. ✓
|
||||
|
||||
### 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.
|
||||
|
||||
### §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). ✓
|
||||
|
||||
---
|
||||
|
||||
## Findings
|
||||
|
||||
### 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.)*
|
||||
|
||||
### 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.
|
||||
|
||||
### 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.)*
|
||||
|
||||
### 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.
|
||||
|
||||
### 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.
|
||||
|
||||
### 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.
|
||||
|
||||
---
|
||||
|
||||
## 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).
|
||||
Loading…
Add table
Add a link
Reference in a new issue