From e202fbef6e5cf36c40ec0cd58b6b95499d7f0277 Mon Sep 17 00:00:00 2001 From: Erik Date: Wed, 12 Aug 2026 05:03:31 +0200 Subject: [PATCH] docs: FA4 mechanism review -- reconcile with the concurrent pass at 6849b457 A second mechanism-lens review landed on the same path at 6849b457 while this one was in progress and was overwritten by 913e35cd. Its text is recoverable from git and is now cited from a new appendix, its two unique findings are carried forward, and the two places the reviews disagree are adjudicated from primary source. Carried forward: - SF-9: AD-78's register row still says "35 of 50 rows dimmed" (the D7 addendum landed in the class doc, not the row's Where column). - N-0: the Open/Close caption does not optimistically pre-toggle; lane B feature 11 records that retail's handler pre-toggles _open_fellow locally before sending 0x0291. Adjudicated: - _ftol2 vs MathF.Round: 6849b457 filed it a NIT ("round and truncation agree on every table value"). That holds for the DECIMAL literals, not the stored floats -- 0x007C91D4 = 0.44999998807907104 and 0x007E72BC = 0.3499999940395355, so retail truncates 44.999998/34.999999 to 44/34 while acdream rounds to 45/35. Stays MUST-FIX 1. - D6 invite auto-response: 6849b457 passed it as verified-clean after confirming the code matches the plan. The binary says retail has no such client-side read on any confirmation path. Stays MUST-FIX 2. The reviews agree on the reconnect D4 hole, the missing panel-level D4 conjunction test, the leader-tint register omission, and the live-DAT probe result. Co-Authored-By: Claude Fable 5 --- .../2026-08-12-fa4-review-mechanism.md | 47 ++++++++++++++++++- 1 file changed, 46 insertions(+), 1 deletion(-) 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.