acdream/docs/research/2026-08-11-op3-rereview.md

Campaign OP slice OP3 — fix-round RE-REVIEW (closure check)

Commit under review: 386076af ("fix(ui): OP3 review fixes — byte-verified Magic chat lines, Gameplay/OptionPage shape, mid-air tri-state, shared geometry") Claims closure of: every finding in docs/research/2026-08-11-op3-review-mechanism.md (M1, S1–S6, N1–N8) and docs/research/2026-08-11-op3-review-blast.md (MUST-FIX 1/2, SHOULD-FIX 1–4, NOTE 1–8). Method: read-only. No build, no test run, no client launch. Git reads and a raw byte read of the PDB-paired retail binary only.

VERDICT: REOPEN (narrow)

The substance of every finding is closed correctly — including M1, whose byte-level claim I re-derived independently and confirmed exactly. Three residuals remain, all cheap, none requiring a design change:

ID	Class	One-line
R1	MUST-FIX	The fix round's own "timestamped, logged" claim is FALSE for acdream (no timestamp prefix is rendered anywhere; no chat-log-file writer exists). It is repeated in the gate script's step 15, so the connected gate now instructs the user to look for something that cannot appear.
R2	SHOULD-FIX	The tri-state IsGrounded does not implement the third state it documents: IsPlayerMode == true with a null controller still returns false → mid-air refusal. Three comments and the commit message assert the opposite — the same comment-vs-code inversion S3 was filed to remove.
R3	SHOULD-FIX	The new dormant-ActivePageChanged pin applies no stimulus, so it asserts silence in a vacuum. It passes identically if dormancy is completely broken.

Everything else is genuinely closed. Details below, evidence first.

---

0. What I verified as CORRECTLY CLOSED

0.1 M1 — INDEPENDENTLY BYTE-VERIFIED, claim exact

The commit claims 6a 07 at 0x0049E972/E9E2/EA52/EAA4/EAF6/EB48.

Internal consistency (the check I was asked for). The mechanism review recorded the six AddTextToScroll call addresses as 0x0049E97C/E9EC/ EA5C/EAAE/EB00/EB52. Every one of the commit's six offsets is exactly 10 bytes below its matching call — a uniform delta, and the first one (0049e972) is verbatim the BN line the review quoted. Re-reading docs/research/named-retail/acclient_2013_pseudo_c.txt:169512-169640 confirms all six *(uint32_t*)((char*)var_2c)[4] = 7; lines carry precisely those six addresses. The recorded evidence is complete and self-consistent.

Independent decode (beyond what was asked; the binary read is read-only). C:\Users\erikn\Downloads\acclient.exe — TimeDateStamp 0x52291F34 = 2013-09-06 00:17:56 UTC, the PDB-paired build; ImageBase 0x400000, .text VA 0x1000 / raw 0x1000, so VA→file offset is identity minus base. Bytes at each of the six sites, with six bytes of lead-in:

6A 00              push 0          ; arg5 windowId   = 0
6A 01              push 1          ; arg4 allowPluginFilter = 1
8D 57 14           lea  edx,[edi+14]
6A 07              push 7          ; arg3 type = 7            <-- THE CLAIM
52                 push edx        ; arg2 text
E8 16 56 0C 00     call 0x00563F90 ; ClientUISystem::GetUISystem
8B C8              mov  ecx,eax    ; this
E8 3F 96 FE FF     call 0x00487FC0 ; ClientSystem::AddTextToScroll(char const*)

All six sites are byte-identical in shape (6A 00 / 6A 01 / 8D {57,4F,47} 14 / 6A 07 / {52,51,50} / call / mov ecx,eax / call). Both call targets resolve by name in the pseudo-C: :368536 00563f90 ClientUISystem::GetUISystem() and :148528 00487fc0 ClientSystem::AddTextToScroll(ClientSystem* this @ ecx, char const* arg2, uint32_t arg3, uint8_t arg4, uint32_t arg5). arg3 (type) is 7 at all six sites. Confirmed.

The code change matches the evidence. RetailUiRuntime.cs:1945-1946 routes the six macro lines through the new OptionsRuntimeBindings.DisplayMouseTurningMacroLine, bound at InteractionRetainedUiComposition.cs:832-833 to AddText(text, RetailLogTextType.Magic); RetailLogTextType.Magic = 0x07 (src/AcDream.Core/Chat/RetailLogTextType.cs:33). The mid-air refusal (RetailUiRuntime.cs:1905) and UA/RA (RetailUiRuntime.cs:1982 → OptionsPanelController.cs:189-192) both still use DisplaySystemMessage = ClientLocal. Exactly the split the review asked for.

Route traced to the chat scroll, no truncation. RuntimeCommunicationState.AddText (:195-203): Trim(), then if (type == ClientLocal) { SpewBox.Enqueue; return; } — Magic falls through to Chat.OnSystemMessage(text, 7). ChatLog.OnSystemMessage (:266-293) appends a ChatEntry with LogTextType = 7; Append (:414-418) trims only against _maxEntries (default 500, :42), so six lines cannot be dropped — the 4-slot SpewBoxState.MaxConcurrentItems truncation that discarded Camera Stiffness and Camera Adjustment is gone. The main window's default type filter is 0xFBFFFFFF (ChatWindowState.cs:111), which clears only bit 0x1A; bit 7 passes. The SystemDedupWindow short-circuit cannot collapse the burst — all six strings are distinct (MouseTurningSettingsMacro.cs:58-104). Colour is #3FBFFF light blue (2026-08-09-chat-retail-color-table.md:151), matching what the gate script tells the user to expect. ✅

(The one thing about this route that is NOT true is the "timestamped, logged" half — see R1.)

0.2 S1 — Gameplay's slot carries no AfterApply; the other three keep theirs

OptionsPanelController.cs:138 _pages.Add(GameplayPageId, new OptionPage { AfterApply = null }); and :139-143 the loop that still gives Character/Chat/Config AfterApply = afterApply. The seam OP4-6 will bind survived intact: Callbacks.AfterApply (:81) → RetailUiRuntime.cs:1983-1984 → Publish(new SaveCharacterOptionsRuntimeCmd()), unchanged, and the three real pages are constructed from it in the same constructor. Nothing about OP4-6's wiring path was narrowed.

Keeping an (empty) OptionPage for the Gameplay slot is one of the two fixes the review itself offered ("or construct it with AfterApply = null and document why"). OnShown()→Apply() over zero rows is then: empty foreach, AfterApply?.Invoke() null, OnOptionChanged?.Invoke() null — byte-for-byte the no-op retail's plain UIElement::OnVisibilityChanged is.

The two tests were rewritten to the right shape. OptionsPanelControllerTests.cs:63-83 ActivateTabs_SelectsGameplayAsDefault_ ButNeverFlushesIt now asserts gameplayFlushCount == 0 with the controller's own AfterApply wired — a real pin, not a rename. :143-159 GameplayPage_OnShownAndOnHidden_NeverFlush_EvenWhenControllerAfterApplyIsWired pins the same property independent of tab framing. The two OptionPageModelTests entries were retargeted at a synthetic empty page and renamed to say so, with the class doc (:5-22) now explicitly stating neither shape models the Gameplay tab.

The model's doc no longer overclaims. OptionPageModel.cs:157-172 keeps the generic empty-page property but bolds "This is a property of the generic empty-page shape, not a description of the Gameplay tab" and cites acclient.h:55857 UIElement_Field. ✅

0.3 S2 — OnOptionChanged fires at retail's four call sites, Defaults ungated

OptionPage.Apply (:222-228) → rows, AfterApply, then OnOptionChanged (retail 0x004F2C95). Reset (:236-241) → changed rows, then OnOptionChanged (0x004F2D4A). Defaults (:246-251) → all rows, then OnOptionChanged (0x004F2CE5). Per-row live edit: BoolOptionRow. SetCurrentValue (:100-105) notifies; RestoreSavedValue/ RestoreDefaultValue deliberately do not — retail's Apply(0) paths, where the owning verb notifies once itself. Wired by OptionPage.Register (:211) through the new IOptionRow.AttachPageNotify. This is exactly the shape the review specified.

Defaults is never gated: the model contains no gate at all, and the OnOptionChanged doc (:191-203) records that retail's own override never fetches 0x100001FE. OnShown/OnHidden inherit the notify through Apply/Reset, matching retail's OnVisibilityChanged → SaveCurrentValues/ RestoreSavedValues, both of which end in OnOptionChanged.

Usable by OP4-6. The seam is Action? OnOptionChanged + the existing bool Changed (:217); a page binds page.OnOptionChanged = () => { apply.Enabled = page.Changed; reset.Enabled = page.Changed; }, which is precisely retail's GetChildRecursive(0x100001FC/FD)

• SetState(0x01/0x0D). The model correctly does NOT own the enabled state itself. Six new tests pin order (["afterApply","onOptionChanged"]), all three verbs, the empty-page case, the SetCurrentValue-only row notify, the Restore* non-notify, and Register's attachment. ✅

0.4 S4/S5/S6 + blast MUST-FIX 2 — register and script bookkeeping

• AD-77 (retail-divergence-register.md:174) is scoped client-wide ("every Mount*/RegisterMainPanel call site for a gmPanelUI sibling … client-wide, not an Options-specific gap"), classed AD not TS, carries the five-column shape with retail anchors, and states the inherited-not- introduced nuance the review asked for. Section count bumped 56 → 57 in the same hunk (:65); TS count correctly not bumped (TS-74 amended, not added). No retired row resurrected in the diff.
• TS-74 (:408) now names all five store-only keys in What/Where (Camera_Stiffness, Camera_AdjustmentSpeed, Camera_AlignToSlope, Input_MouseLookSensitivity, Input_InvertMouseLookYAxis), cites CameraTurningSettings.cs and CameraPointerInputController.cs as the two separate stores, and adds the "chat line quotes an unrelated stored value" consequence to Risk. Exactly S5's ask.
• Test script: step 3 gains the ghosted→enabled + highlight check; step 5 is corrected to bottom-edge-only with an explicit "this is a correction from OP3-as-landed" note; steps 13/14 gain the UA/RA legibility eye-item (blast SHOULD-FIX 4); steps 16/17 add relogin (0x0005 survived) and relaunch (five prefs survived), both written so a failure is discriminating rather than ambiguous; "What to report" updated to 17 steps. ✅

0.5 blast SHOULD-FIX 2 — geometry now matches the cohort exactly

RetailUiRuntime.cs:2021-2023 ResizeX = false, ResizeY = true, ResizableEdges = ResizeEdges.Bottom, with MinWidth/MinHeight removed. I read every sibling that calls RegisterMainPanel: spellbook (:1337-1339), effects (:1528-1530), the four indicator-detail panels (:1751-1753), character (:2266-2268), inventory (:2349-2351) — all identical. No ResizeX/min-size divergence remains. ContentAnchors all-four matches effects and the indicator-detail panels, so it is not a new outlier either. ✅

0.6 Blast radius OF the fixes — clean on all three axes

• Typed message seam: OptionsRuntimeBindings has exactly ONE construction site (InteractionRetainedUiComposition.cs:801) and DisplayMouseTurningMacroLine exactly ONE consumer (RetailUiRuntime.cs:1946). No test constructs the record, so the added positional parameter breaks nothing.
• Func<bool> → Func<bool?>: one production caller (RetailUiRuntime.cs:1899); the switch covers true/false/null exhaustively. The unrelated IsGrounded occurrences across Runtime/Core.Net are UpdatePosition.PositionFlags.IsGrounded — a different symbol entirely.
• IOptionRow.AttachPageNotify: BoolOptionRow is the ONLY implementer (no test fakes), so widening the interface compiles everywhere.
• Geometry: no test pins the old Options geometry. The only Options geometry in tests is the new RetailPanelUiControllerTests helper mount. A stale persisted width from the pre-fix build is also harmless — I traced it: RetailWindowLayoutPersistence.Apply:213 → RetailWindowHandle.ResizeTo:69-70 → RetailWindowManager.ResizeTo:184 if (!frame.ResizeX) width = frame.Width;. The saved width is discarded on restore. Verified non-issue.

0.7 Two of the three new pins genuinely pin

• RetailPanelUiControllerTests.cs:131-148 uses the REAL RetailPanelCatalog.Options (10) / .Character (11) ids against a real RetailPanelUiController.RegisterMainPanel, and asserts the show-hides-the- other transition in both directions plus ActivePanelId. Genuine — it would fail if SetPanelVisibility's exclusion regressed.
• RetailDialogFactoryTests.cs:95-110 exercises the queueing end-to-end (ActiveCount == 1, PendingCount == 1), which really would break to 2/0 if the omitted-key overload took a different key. Slightly tautological at the language level (the default IS DefaultQueueKey in the signature, RetailDialogFactory.cs:128), but the behavioural assertion is real.
• The third does not pin — see R3.

---

1. RESIDUALS

R1 — MUST-FIX: "timestamped, logged" is retail's behaviour, not acdream's, and the gate script now tells the user to look for it

The claim, in three places:

1. src/AcDream.App/UI/RetailUiRuntime.cs:190-192 (XML doc on DisplayMouseTurningMacroLine): "Magic routes through RuntimeCommunicationState.AddText to the scrolling chat transcript (light blue, timestamped, logged)".
2. The commit message: "routed at Magic (scrolling chat transcript, light blue, timestamped)".
3. docs/research/2026-08-11-campaign-op-test-script.md, step 15: "expect LIGHT BLUE text in the SCROLLING CHAT TRANSCRIPT (not the transient SpewBox overlay), with a timestamp prefix, same as any other Magic-colored system line."

acdream renders neither. Evidence:

• ChatEntry.Received exists (src/AcDream.Core/Chat/ChatLog.cs:457) but has zero consumers — git grep "\.Received" -- src/ returns only NetClient.cs/FragmentAssembler.cs hits on unrelated ReceivedBytes/ ReceivedCount members. Nothing formats it into a line.
• ChatSettings.ShowTimestamps (src/AcDream.UI.Abstractions/Panels/Settings/ChatSettings.cs:46) is a stored preference whose only references are its own declaration, its default (:107), the unrendered ImGui-era SettingsPanel.cs:415-417, and SettingsStore.cs:196/597. No renderer reads it.
• There is no chat-log-file writer at all: git grep -ni "chatlog\.txt|WriteChatLog|LogChatToFile" -- src/ returns nothing, and register row TS-69 records @log as unported precisely because "needs a safely-lifecycled chat-to-file writer (deferred)".

Why MUST. Item 3 corrupts the acceptance instrument this whole fix round exists to make meaningful. The blast review's own MUST-FIX rationale was that M1 "changes what the gate is supposed to observe"; a gate step that asks the user to confirm a prefix acdream cannot draw will produce either a false failure report or, worse, a follow-up "fix" for a non-bug. Item 1 is the same class of defect S1 just retired in this very commit: a doc sentence naming an acdream type and method while describing retail's mechanism.

Fix (documentation only, no code): in step 15 drop "with a timestamp prefix"; in RetailUiRuntime.cs:190-192 either drop "timestamped, logged" or re-attribute it ("retail additionally timestamps and log-files this type; acdream does neither — see TS-69"). Nothing about the Magic routing itself changes.

R2 — SHOULD-FIX: the tri-state does not implement its own third state

src/AcDream.App/Composition/InteractionRetainedUiComposition.cs:814-816:

IsGrounded: () => !d.PlayerMode.IsPlayerMode
    ? (bool?)null
    : d.PlayerController.Controller is { IsAirborne: false },

Enumerating the states as asked:

State	Expression	RequestExitToCharacterSelection	Retail	✓
player mode, controller live, grounded	true	EndCharacterSession()	proceed	✅
player mode, controller live, airborne	false	"Cannot log off while in mid-air."	refuse	✅
not player mode	null	silent	silent	✅
player mode, controller null	false	refusal	silent	❌

The fourth row is the miss. Controller is { IsAirborne: false } is false for a null controller, so it lands in case false: (RetailUiRuntime.cs:1904-1906) and emits the refusal.

Three comments assert the opposite of the code — the exact inversion S3 was filed to remove:

• InteractionRetainedUiComposition.cs:807-808: "with no player object (not in player mode, or no live controller yet) neither the refusal nor the logoff itself ever fires, i.e. a SILENT no-op, not a refusal."
• RetailUiRuntime.cs:161-163: "null means 'no player' (acdream: !d.PlayerMode.IsPlayerMode || d.PlayerController.Controller is null)".
• RetailUiRuntime.cs:1908-1911: "outside player mode (or with no live controller) there is no player object for UseTime to test at all".

Plus the commit message's own "(or with no live controller)".

Reachability is narrow but the two owners are explicitly independent. IsPlayerMode is App-owned and set true only after the controller is in hand (PlayerModeController.cs:370, after controller.Position is read at :362), and Exit() clears it at :166. But PlayerModeController.cs:167-169 states outright: "the movement controller is Runtime-owned — player-mode exit detaches presentation only; the publication lifecycle (generation reset/ teardown) owns the controller's retirement." Two owners, two clocks. Any ordering in which Runtime clears the controller before App clears IsPlayerMode reaches row four.

Fix (one expression):

IsGrounded: () => !d.PlayerMode.IsPlayerMode
        || d.PlayerController.Controller is null
    ? (bool?)null
    : d.PlayerController.Controller is { IsAirborne: false },

which is literally what the XML doc already says.

On the "no dedicated automated test" flag: for the switch itself, acceptable — RetailUiRuntime is composition-driven and a direct unit test would need a UiRoot plus DAT layouts, and there are zero test constructions of OptionsRuntimeBindings today. But note what the absence cost here: an untested expression silently failed to implement its own documented contract. If R2 is fixed, the cheapest real pin is a one-line test over the lambda shape rather than the runtime — or simply leaving it, given the fix makes expression and doc agree by construction.

R3 — SHOULD-FIX: the dormant-ActivePageChanged pin applies no stimulus

tests/AcDream.App.Tests/UI/Layout/OP2ReworkBlastRadiusConformanceTests.cs:83-93:

var host = Assert.IsType<UiTabPanel>(layout.FindElement(0x100000B8u));
int raised = 0;
host.ActivePageChanged += (_, _) => raised++;

Assert.False(host.BehaviorActive);
Assert.Equal(0, raised);

Nothing between the subscription and the assertion does anything. The test would pass identically if the dormancy model were entirely broken, because no switch is ever attempted. It adds only the subscription over the test directly above it (:61-70), which already asserts BehaviorActive == false and ActivePageElementId == 0.

The parent's framing is right and the answer is the weaker variant shipped. Worth being precise about what stimulus is correct, because "call SwitchTo and assert silence" would legitimately fail: UiTabPanel.SwitchTo has no dormancy guard and raises unconditionally at its tail (src/AcDream.App/UI/UiTabPanel.cs, ActivePageChanged?.Invoke(...) as the last statement). Dormancy lives entirely in the click bindings: ActivateTabBehavior is the only caller of RetailTabBinding.SetClick(button, () => SwitchTo(pageId)), and RetailTabBinding.SetClick (RetailTabBinding.cs:12-23) sets button.OnClick = action.

Fix: drive the authored tab button on a dormant host —

var button = layout.FindElement(host.Tabs[0].ButtonElementId);
Assert.Null(((UiButton)button!).OnClick);   // never wired -> the real mechanism
Assert.Equal(0, raised);
Assert.Equal(0u, host.ActivePageElementId);

That fails the moment eager wiring returns, which is the regression AD-73 exists to prevent.

---

2. NOTES (no action required)

• N1 — EmptyPage_WithNoAfterApplyWired_Apply_NeverFlushes (OptionPageModelTests.cs:90-101) asserts only Assert.Null(thrown). The name promises "never flushes"; with AfterApply null there is nothing to count, so it is really a null-safety test. Harmless because the real S1 pin lives in OptionsPanelControllerTests.cs:143-159 and does count. Rename or leave.
• N2 — plan §5 still open-ended. docs/plans/2026-08-10-options-panel-campaign.md:381 still reads "register row in OP3 if the review deems it a divergence" with no pointer to AD-77. A future reader re-litigates a settled ruling. One-line edit whenever that file is next touched.
• N3 — BoolOptionRow.AttachPageNotify is a single-slot assignment (OptionPageModel.cs:107). Registering one row into two pages silently drops the first page's notify. This matches retail's single UIOption::m_pOCH pointer, so it is faithful, not a bug — recorded so OP4-6 does not share a row instance across pages expecting both to hear it.
• N4 — Options can now only GROW. With MinWidth/MinHeight removed and no DatConstraintSource passed, RetailWindowFrame.ResolveConstraint:187-201 falls through to the fallback argument, which is the mounted outer extent (:161-164) — i.e. min = the authored 300 x 362 plus chrome inset. That is identical to all nine siblings, so it is the correct outcome, but gate-script step 5 says only "resizable from the bottom edge only … remembers its HEIGHT" without noting the window cannot be dragged shorter than it opens. A tester may report that as a defect. One clause would pre-empt it.
• N5 — OptionsPanelController.Pages' doc (:104-109) still describes the four slots uniformly ("Every entry exists from construction … only GameplayPageId's stays permanently empty") without mentioning the AfterApply asymmetry. The constructor comment immediately below covers it. Cosmetic.
• N6 — test counts unverified. The commit's "12,947 passed / 4 skipped / 0 failed" was not re-run; this re-review is read-only by instruction.

---

3. Closure table

Finding	Claimed	Actually
M1 (both lenses)	closed, byte-verified	CLOSED — independently re-decoded; 6a 07 at all six sites, arg3 of ClientSystem::AddTextToScroll @0x00487FC0. Route traced to the chat scroll, filter passes, no truncation, no dedupe collapse. R1 is a doc/gate overclaim attached to it, not a routing error.
S1	closed	CLOSED — Gameplay slot AfterApply = null; other three keep the real seam; both tests rewritten to the right shape; model doc corrected.
S2	closed	CLOSED — notify at all three verb tails + SetCurrentValue only; Defaults ungated; seam usable by OP4-6.
S3 / blast SHOULD-FIX 1	closed	PARTIAL → R2. Three of four states right; IsPlayerMode && Controller == null still refuses, and three comments plus the commit message say it does not.
S4 / blast MUST-FIX 2	closed	CLOSED — AD-77 filed, client-wide, AD-classed, count 56→57, format correct.
S5	closed	CLOSED — TS-74 What/Where names all five keys and both stores.
S6	closed	CLOSED — all three script additions present and discriminating (but see R1 on step 15).
blast SHOULD-FIX 2 (geometry)	closed	CLOSED — matches all nine siblings exactly; stale persisted width provably discarded on restore.
blast SHOULD-FIX 3 (three pins)	closed	2 of 3 CLOSED → R3. Mutual exclusion and queue key genuinely pin; dormant ActivePageChanged does not.
blast SHOULD-FIX 4 (UA/RA legibility)	closed	CLOSED — eye-item added to steps 13/14 with the honest-fix direction named.
blast NOTE 8 (close-button logging)	closed	CLOSED — OptionsPanelController.cs:176-179, symmetric with BindButton.
New blast radius from the fixes	—	CLEAN — one construction site for the new seam, one caller for the changed signature, one IOptionRow implementer, no test pins the old geometry.