# 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` → `Func`:** 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`: ```csharp 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):** ```csharp 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`: ```csharp var host = Assert.IsType(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 — ```csharp 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. |