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

Campaign OP slice OP4 — fix-round RE-REVIEW

Commit under review: bc43fb1d "fix(ui,runtime): OP4 review fixes — live re-seed, enable-gating, Combat panel re-point, universal timestamps" Claim under test: closure of every consolidated finding from docs/research/2026-08-11-op4-review-mechanism.md (MF-1, MF-2, SF-1, SF-2, SF-3, N-1..N-5) and docs/research/2026-08-11-op4-review-blast.md (M1, M2, M3, S1–S6, N1–N8). Mode: read-only. No build, no test run, no client launch. The only file written is this one.

---

Verdict

REOPEN — narrow. Nine of the ten consolidated findings are genuinely closed, several of them better than the reviews asked for. One fix (S1/SHOULD-FIX, universal chat timestamps) introduced a NEW defect that is worse than the gap it closed: the prefix is applied to ChatEntry.Text — the raw message body — instead of to the composed display line, so every sender-formatted chat kind now renders the timestamp inside the quoted speech:

Alice says, "13:05:09 hello"      ← acdream after bc43fb1d
13:05:09 Alice says, "hello"      ← retail

Six of the ten ChatKinds are affected (LocalSpeech, RangedSpeech, Channel, Tell, Emote, SoulEmote); the four that happen to be correct (System, Combat, Popup, and the AddText callers) are exactly the ones that were already covered before this commit. The new tests pin the wrong invariant, so the suite is green on a broken line format. Gate-script step 13 will fail on the "inbound tell/say" half it was written for.

This is one localized fix (move the prefix to the composed-line seam) plus a test rewrite. Nothing else in the round needs to change.

Class	Count	Items
MUST-FIX	1	R1 — timestamp prefixes the message body, not the composed line
SHOULD-FIX	3	R2 — N-5 (server-echo refresh) neither wired nor registered, and this commit widened its blast; R3 — the refresh widget push has no test; R4 — headless AutoRepeatAttack default flips false → true, unnamed in AP-196
NOTE	3	R5 — refresh-from-SaveCurrentValue is an adaptation, not retail's shape (comment reads otherwise); R6 — enable-gate tests cover the Character page only; R7 — HeadlessGameplayOperations combat options can now throw where they were constants

---

Per-finding closure check

MF-1 / blast M1 — stale pre-login row seed — CLOSED

Traced end to end.

The re-seed mechanism. BoolOptionRow gained read/refresh delegates (src/AcDream.App/UI/Layout/OptionPageModel.cs:86-99) and SaveCurrentValue is now (src/AcDream.App/UI/Layout/OptionPageModel.cs:148-156):

if (_read is not null) { _current = _read(); _refresh?.Invoke(_current); }
_saved = _current;

Retail's UIOption_Checkbox::SaveCurrentValue @0x004868e0 is verbatim m_current = vtable->GetValue(); m_saved = m_current; (pseudo-C :146936), and UIOption_Checkbox::GetValue @0x00486f60 (:147401) resolves to PlayerModule::GetOption(m_playerOption) at 00486ff8 — the live server-synced word, exactly as the mechanism review claimed. The wiring supplies the live reader and a widget push that deliberately bypasses apply (CharacterOptionsPageController.cs:383-385: read: () => bindings.CurrentValue(spec.Id), refresh: value => checkbox.Selected = value), so a re-seed cannot echo back onto the wire. UiButton.Selected's setter only calls UpdateVisualState() (UiButton.cs:162-171) — no Click, no OnClick — so the push is inert on the wire by construction, not just by convention.

The show path. RetailWindowFrame.Mount(... Visible = false ...) (RetailUiRuntime.cs:2064) means no OnShown fires at composition, so the pre-login mount no longer matters. Opening the panel sets the frame visible → UiRoot.ElementVisibilityChanged → RetailWindowManager.OnElementVisibility Changed → handle.NotifyVisibility(true) (RetailWindowManager.cs:276) → Controller.OnShown() (RetailWindowHandle.cs:101) → OptionsPanelController.OnShown() (:307-311) → active OptionPage.OnShown() → Apply() → SaveCurrentValue() on all 50 rows. Tab-switch-in takes the same path through OnActivePageChanged (:283-295). This matches retail's PlayerOptionPage::OnVisibilityChanged(true) → SaveCurrentValues (pseudo-C :245374, :245757).

The exact blast-M1 scenario now behaves correctly. Server ON, client constructor default OFF, panel shown, row clicked (now a real toggle to OFF, not a silent no-op), tab switched away → OnHidden → Reset → the row is Changed → RestoreSavedValue() restores _saved, which the show re-seeded to true, and apply(true) re-asserts the server's own value. The wrong-direction mutation is gone. Pinned by Reset_AfterReseed_RestoresTheLiveValue_NotTheStaleConstructionDefault (tests/AcDream.App.Tests/UI/Layout/CharacterOptionsPageControllerTests.cs), plus a 50-row convergence test and a "no SetOption during re-read" assertion.

The read closure stays valid across sessions. CurrentCharacterOption is id => d.Character.Options.GetOptionBit(id) (InteractionRetainedUiComposition.cs:860); RuntimeCharacterState.Options is a get-only property assigned once (RuntimeCharacterState.cs:105,118), and reconnect calls Options.ResetSession on that same instance (:442) rather than replacing it — so the retained UI's captured delegate never goes stale.

LockUI convergence. OnCharacterOptionsChanged now also pushes the live bit through SetUiLocked (LiveSessionRuntimeFactory.cs:336-341). It fires from LiveSessionEventRouter.cs:210-226 on every complete PlayerDescription — login and reconnect — and deliberately not on a trailer-truncated parse (OP7 MF-2, correct). Threading is safe: the router runs under WorldSession.Tick(), the per-frame pump (WorldSession.cs:1117-1123), i.e. the same thread the retained UI runs on, so the new UiRoot.UiLocked write and its UiLockChanged fan-out (RetailWindowManager.cs:316-320, which enumerates the window dictionary) are not a cross-thread hazard.

MF-2 — Apply/Reset enable-gating — CLOSED

OptionsPanelController.Bind now captures both buttons and installs the gate per page (:220-243), running it once at bind time for retail's PostInit → OnOptionChanged(0) tail; Defaults is wired but never gated. Verified against the committed fixture that this actually works on all three pages: each of the nine 0x100001FC/FD/FE instances authors a Ghosted state and does not carry DAT property 0x0B (ToggleBehavior). That second fact is load-bearing and easy to miss — UiButton.TrySetRetailState short-circuits Normal/ Highlight into a Selected flip for toggle buttons without re-enabling (UiButton.cs:212-216), which would have left Apply/Reset permanently ghosted after the first Apply. They are push buttons, so the Ghosted → Enabled=false / !Enabled → Enabled=true path (:217-223) is the one taken. UiElement.Pick skips !Enabled subtrees (UiElement.cs:586) and UiButton.OnEvent's Click case returns early when !Enabled (:494), so a ghosted button is genuinely unclickable. Five tests pin start-disabled, click-enables, Apply-disables, Defaults-leaves-enabled, Defaults-never-gated.

M2 — two writable copies (Combat panel) — CLOSED

The retirement is complete, not gestured. grep across src/ and tests/ finds zero readers or writers of GameplaySettings.{AutoTarget, AutoRepeatAttack, ViewCombatTarget}, RuntimeSettingsController.{AutoTarget, AutoRepeatAttack, ViewCombatTarget, SetCombatGameplay}, or GameplaySettingsState — all deleted, including the settings.json read/write keys (SettingsStore.cs) and the dead ImGui SettingsPanel checkboxes. The ICombatGameplaySettingsSource interface survives with CharacterOptionCombatSettingsSource as its only implementation.

Convergence trace: the Combat LED click → CombatUiController.Bindings.SetOption (CombatUiController.cs:127-135) → SetSingleCharacterOptionRuntimeCmd (RetailUiRuntime.cs:1203-1206) → LiveSessionCommandRouter → SendSingleCharacterOption → RuntimeCharacterOptionsState.TrySetOption (RuntimeCharacterState.cs:806). The Character tab's row publishes the identical command. Reads are GetOptionBit on both sides (CombatUiController.SyncControls :209-211; BoolOptionRow._read). One store, one writer path — the divergence is closed. They converge on the next refresh of whichever surface was not clicked (SyncControls runs on OnShown/combat-mode/attack-state changes), which the gate script's new step 20 states honestly ("toggling one and reopening/refreshing the other"). See R2 for the residual.

HeadlessGameplayOperations now reads the live bits (src/AcDream.Headless/Hosting/HeadlessGameplayOperations.cs:135-138), closing blast S5. AP-196 (docs/architecture/retail-divergence-register.md:189) records the default-source change, the ViewCombatTarget true→false flip, the three-field deletion, and the "other five stay as write-behind mirrors" distinction truthfully; the AP header count moved 136 → 138 (:178).

M3 — SetUiLocked convergence guard — CLOSED

_lastAppliedUiLocked (RuntimeSettingsController.cs:188) is a bool? starting null, compared instead of Gameplay.LockUI (:361) and written only after the runtime target actually accepted the push (:366-367), with the same bookkeeping added to SaveGameplay's own push (:525-526).

Walking the blast-M3 scenario — persisted "lockUI": true, fresh login, one toggle press: login's OnCharacterOptionsChanged calls SetUiLocked(false) (server bit clear); null != false → applies, _lastAppliedUiLocked = false. The toggle then computes locked = !GetOptionBit(LockUI) = true (LiveSessionRuntimeFactory.cs:413-418); false != true → ApplyUiLock runs. The old guard would have early-returned on the first press. Pinned by SetUiLocked_AppliesEvenWhenGameplayLockUIAlreadyMatches_IfNeverActuallyApplied, which also asserts the second identical call correctly no-ops.

S1 — DisplayTimeStamps coverage — NOT CLOSED (regressed): see R1

Coverage is fixed — ChatLog.Append is a true chokepoint (one _buffer.Enqueue, ten producers, all funnelling through it), and the three bypass paths spot-checked (heard speech via OnLocalSpeech, Turbine channels via OnChannelBroadcast, combat text via OnCombatLine) now all get a prefix. The SpewBox stays exempt for free (RuntimeCommunicationState.AddText:228-232 returns before touching Chat), and moving the prefix behind OnSystemMessage's dedupe comparison actually retires blast N5's dedupe concern. But the placement is wrong — full detail in R1.

S2 — sky fog gate — CLOSED

sky.frag:68-69 now reads int fogMode = int(uFogParams.w); if (uApplyFog > 0.5 && fogMode != 0), matching mesh_modern.frag/terrain_modern.frag's mode == 0 convention; uFogParams is already in the frag shader's UBO (sky.frag:50).

.spv hygiene claim verified. git show --stat for src/AcDream.App/Rendering/ lists exactly sky.frag, sky.frag.spv, one manifest line, and GameWindow.cs's comment edit — no collateral shader churn. And the manifest hash is genuinely correct: recomputing the compiler's own scheme (tools/ShaderCompiler/Program.cs:214-220 — UTF-8 of the source with \r\n → \n) over sky.frag yields 2ddf210d69b0c4a3c0870eecfb0ccba2097d93b365729739402bfe91e3b120d0, exactly the new manifest value (cross-checked against mesh_modern.frag's untouched entry to confirm the scheme).

S4 / SF-1 — culture-invariant format, doc downgrade, AP-197 — CLOSED

FormatTimestampPrefix (ChatLog.cs:468-469) is DateTime.Now.ToString(@"H\:mm\:ss ", CultureInfo.InvariantCulture) — colons escaped so they are literals rather than DateTimeFormatInfo.TimeSeparator, matching the CRT wcsftime(u"%#H:%M:%S ") retail actually calls (pseudo-C 00563e24). A fi-FI test exists at both layers. The "byte-verified" overclaim is downgraded to "BN-sourced … NOT byte-verified — wire doc U6" in both doc comments, and AP-197 (retail-divergence-register.md:190) records the ignored per-character GenericQualitiesData key-1 format override with an accurate risk column. N-4's AP-194 anchor-column typo is fixed (:186, UIOption_Checkbox:: SetPlayerOption @0x00486e80, with the correction annotated). N-7's RunAsDefaultMovement prose now cites ToggleRun_PlayerOption at all three sites.

SF-2 / SF-3 / S6 — tests and gate script — CLOSED (with R3, R6)

• Six header keys hash-pinned by [Theory], each also asserted present in Groups (so a rename can't quietly orphan the pin).
• A real MouseDown/MouseUp/Click sequence on the built 0x10000219 checkbox, asserting both Selected and the published SetOption — this pins the ToggleBehavior dependency that all 50 LEDs rest on.
• The option × modifier truth table (4 rows) plus AutoRunActive with the option OFF, the latter honestly labelled a current-behavior pin rather than a retail-correctness claim (N6 stays open as a question, correctly).
• Gate script step 8 now states the logout-flush mechanism and both outcomes; new steps 19 (enable-gating) and 20 (Combat/Character cross-check) exist and are worded to match what the code actually does.

Not verifiable read-only: the commit's "13,044 passed / 4 skipped" claim.

---

Residuals

R1 — MUST-FIX — the timestamp prefixes the message body, not the composed line

ChatLog.Append (src/AcDream.Core/Chat/ChatLog.cs:436-444) rewrites entry.Text:

if (DisplayTimestampsSource?.Invoke() == true)
    entry = entry with { Text = FormatTimestampPrefix() + entry.Text };

But ChatEntry.Text is the message body, not the display line. The display line is composed later, from Sender + Text, in ChatVM.FormatEntry (src/AcDream.UI.Abstractions/Panels/Chat/ChatVM.cs:251-300) — the same method both retained chat windows render through (ChatWindowController.cs:681 and FloatingChatWindowController.cs:272, via RecentLinesDetailed() at ChatVM.cs:334-351). Result, per kind:

Kind	Rendered after bc43fb1d	Retail
LocalSpeech / RangedSpeech	Alice says, "13:05:09 hi"	13:05:09 Alice says, "hi"
Channel	[Trade] Bob says, "13:05:09 wts"	13:05:09 [Trade] Bob says, "wts"
Tell	Regal tells you, "13:05:09 psst"	13:05:09 Regal tells you, "psst"
Emote / SoulEmote	* Caith 13:05:09 waves	13:05:09 * Caith waves
System / Combat / Popup	correct (Text is the line)	correct

Retail is unambiguous here, in two independent places. ClientSystem::AddTextToScroll @0x00563c50 (pseudo-C :368347) receives an already-composed line in arg2, formats the timestamp into a separate string (00563e24 wcsftime(... u"%#H:%M:%S " ...) → s_NullBuffer_4, which is initialized to the empty buffer at 00563dcd), and passes the two as distinct StringInfos to ECM_UI::SendNotice_DisplayFinalStringInfo(arg3, &text, &timestamp, arg5) at 00563f2b; the log-file branch writes fprintf(..., "%ls%ls\n", timestamp, text) at 00563e5b — timestamp first, whole line second. And the caller side confirms the line is composed before the call: ClientCommunicationSystem::Handle_Communication__HearSpeech builds sprintf("%s %s\n", speakerPhrase, text) at 00571533 and only then calls AddTextToScroll at 0057154d (pseudo-C :382204ff).

The new tests encode the defect rather than catching it. ChatLogTests.DisplayTimestampsSource_AppliesToEveryProducer (:381) and RuntimeCommunicationStateTests.DisplayTimestampsSource_ForwardsToChat_ TimestampingEveryProducer_NotJustAddText (:314) both assert ^\d{1,2}:\d{2}:\d{2} hi$ on the raw ChatEntry.Text of a LocalSpeech entry — the exact string that will render inside the quotes. No test asserts a rendered line, so the suite cannot go red on this.

Consequence for the gate. Script step 13 asks the user to "send/receive a new chat line (e.g. /who or any inbound tell/say)" and expects a prefix. The /who half (System) passes; the tell/say half shows the timestamp inside the quotation marks. This burns a gate round on a known-wrong line format.

Fix shape. Prefix at the composed-line seam, not the entry: apply it in ChatVM.FormatEntry/RecentLinesDetailed (retail's own model is a separate leading field, so a render-time prefix is the faithful analog and also keeps the stored transcript clean for the runtime event stream). Retarget the tests onto FormatEntry output for at least one sender-composed kind per shape (speech, tell, channel, emote) plus one bare kind (System), and keep the culture test at whichever layer owns the formatting.

R2 — SHOULD-FIX — N-5 (no refresh on the server's own option echo) is still neither wired nor registered, and this commit widened its blast radius

Mechanism review N-5 said: wire OnCharacterOptionsChanged to refresh the visible page, or file a register row. Neither happened. grep finds no register row mentioning SendNotice_RefreshOptionsPanel / SendNotice_PlayerOptionChanged / a deferred options refresh.

This is no longer purely cosmetic after bc43fb1d, because the commit added a second surface reading the same store:

1. Cross-panel disagreement. With the Combat window open, clicking "Automatically Repeat Attacks" on the Character tab changes the bit but the Combat LED keeps its old art until the next SyncControls (CombatUiController.cs:228 OnShown, combat-mode change, or attack-state change) — and vice versa. Both now read one store, so this is a stale repaint, not a stale value; it is nonetheless a new user-visible inconsistency introduced by this commit.
2. Panel open across the seed. MF-1 re-seeds on show/tab-switch only, not on the PlayerDescription seed itself — the very hook this commit wired for LockUI (LiveSessionRuntimeFactory.cs:336-341). A Character tab that is already open when the seed lands keeps constructor-default rows and constructor-default _saved baselines, which reopens the M1 "Reset writes the wrong direction" failure for that window. Same for a reconnect (Options.ResetSession → fresh PlayerDescription) with the panel left open.

Cheapest closure: have OnCharacterOptionsChanged also call OptionsPanelController.OnShown()-equivalent re-seed on the visible page (the hook is already wired and already proven safe on this thread), and call CombatUiController.SyncControls() from the same place. Otherwise, file the register row N-5 asked for and say which slice owns it.

R3 — SHOULD-FIX — the refresh widget push has no test

MF-1's stated fix instruction was to assert "the row and the checkbox both converge on the next OnShown()". The three new re-seed tests assert row.Current / row.Saved only; no test reads checkbox.Selected after a re-seed (the only checkbox.Selected assertions in the file are in the new click test, CharacterOptionsPageControllerTests.cs:630,636). The refresh delegate — the half that makes the LED art match the re-read value, and the half a future refactor is most likely to drop — is production-only. One assertion on the first row's checkbox inside OnShown_ReSeedsRow_FromLiveBindingValue_ChangedBehindItsBack closes it.

R4 — SHOULD-FIX — headless AutoRepeatAttack's observable default flips false → true, unnamed in AP-196

HeadlessGameplayOperations.AutoRepeatAttack was the literal false; it now reads the live bit (:135-136), whose pre-seed value is the retail constructor default ON (CharacterOptionTable.cs:111, Add(AutoRepeatAttack, true, 0x00000002u, true, true) — auto-save, client default true). Every existing headless bot that never configured characterOptions therefore starts auto-repeating attacks where it previously did not. That is retail-correct and desirable, but it is an observable-default change of exactly the class AP-196 was filed to record, and AP-196 names only ViewCombatTarget's flip. Add the sentence (or a Slice-K-facing note) so a future "why is my bot chain-attacking?" report lands on the row instead of on an investigation.

R5 — NOTE — refresh-from-SaveCurrentValue is an acdream adaptation, and the doc comment reads as if it were retail's shape

Retail's SaveCurrentValue @0x004868e0 does not call Refresh() — only RestoreSavedValue @0x00486900 and RestoreDefaultValue do (pseudo-C :146936, :146950). Retail does not need it: its options word is seeded before the panel's rows exist, so m_current can never drift from the art. acdream's re-read can change _current with no other write, so the push is required — a justified adaptation. The refresh param doc (OptionPageModel.cs:79-85) opens "Retail's own Refresh() push of the re-read value onto the widget … invoked ONLY from SaveCurrentValue's re-read path", which a future reader grepping 0x004868E0 will find contradicted by the decomp. One clause ("acdream-only — retail's SaveCurrentValue needs no push because its rows are built after the seed") removes the trap.

R6 — NOTE — the enable-gate tests cover the Character page only

GetCharacterPageButtons scopes to 0x10000211; the Chat (0x1000050C) and Config (0x10000213) pages get the same gate installed by the same loop but are unpinned. The fixture confirms all nine buttons have the required shape (no 0x0B, Ghosted present), so this is coverage, not a defect — but a one-line [Theory] over the three page ids would also pin the per-page scoping the slice's own commit message calls out.

R7 — NOTE — HeadlessGameplayOperations's combat options can now throw

AutoRepeatAttack/AutoTarget call RequireRuntime(), which throws InvalidOperationException when unbound (:272-275); they were constants and could never throw. _runtime is assigned once in Bind and never cleared (:46,65), and both properties are only read from runtime-attached combat flows, so this is not reachable today. Recorded so a future teardown-ordering change doesn't rediscover it as a crash.

---

Blast radius of the fixes themselves (checked, clean except R1/R2/R4)

• ChatLog.DisplayTimestampsSource is a new public property on a Core type; the only binder is GameWindow.cs:662 (graphical host). Headless never binds it — HeadlessGameplayOperations's only chat write (:252 CommunicationOwner.Chat.OnSystemMessage) goes through the same Append but with the source null, so bot event streams and the 67 Headless tests are untouched. No plugin-abstraction surface references ChatLog or ChatEntry. RuntimeCommunicationState.DisplayTimestampsSource became a forwarding property, so every existing caller compiles unchanged.
• CombatUiController.Bindings rewiring — exactly one production construction site (RetailUiRuntime.cs:1199-1206) and one CombatRuntimeBindings site (InteractionRetainedUiComposition.cs:644-646), both updated; OptionsRuntimeBindings likewise has a single construction site, so MountCombat's new _bindings.Options dependency cannot be null. Tests construct through a fake with the same shape.
• Deleted RuntimeSettingsController members — zero remaining references anywhere in src/ or tests/. GameplaySettings's three positional parameters were removed, which is a compile-time forcing function (AP-196 says so explicitly); stale autoTarget/autoRepeatAttack/viewCombatTarget keys in an existing settings.json are simply ignored on read and dropped on write.
• SetUiLocked on every seed now persists a server-derived lockUI into settings.json and updates the Settings VM draft from the session route. That is one save per session start (the guard no-ops afterwards), on the same thread as the existing SyncChatFromServerOptions call beside it, and it makes the persisted value track the server — consistent with AP-196's "write-behind mirror" framing. No re-entrancy: ApplyUiLock terminates at UiRoot.UiLocked = locked (RuntimeSettingsTargets.cs:192) and never publishes a wire command.