diff --git a/docs/research/2026-08-11-op4-rereview.md b/docs/research/2026-08-11-op4-rereview.md new file mode 100644 index 00000000..1333416b --- /dev/null +++ b/docs/research/2026-08-11-op4-rereview.md @@ -0,0 +1,425 @@ +# 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 `ChatKind`s 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`): + +```csharp +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`: + +```csharp +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 +`StringInfo`s to `ECM_UI::SendNotice_DisplayFinalStringInfo(arg3, &text, +×tamp, 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 `:382204`ff). + +**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.