diff --git a/docs/research/2026-08-12-fa4-review-mechanism.md b/docs/research/2026-08-12-fa4-review-mechanism.md index e0f03b95..f7ab6a5d 100644 --- a/docs/research/2026-08-12-fa4-review-mechanism.md +++ b/docs/research/2026-08-12-fa4-review-mechanism.md @@ -10,7 +10,7 @@ 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.** +**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 @@ -442,10 +442,31 @@ 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 @@ -502,3 +523,27 @@ reader from assuming atomicity. 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.