# OP4 review — regression / blast-radius lens

**Commit under review:** `22b86b9f` "feat(ui): Campaign OP slice OP4 — the Character tab"
**Reviewer lens:** regression + blast radius (both axes on every changed surface)
**Method:** read-only. Diff walk, pre/post consumer enumeration by `git grep` at
`22b86b9f^` and `22b86b9f`, fixture-tree analysis of
`options_panel_2100006E_1000018D.json`, decomp cross-check against
`docs/research/named-retail/acclient_2013_pseudo_c.txt` and `acclient.h`.
No build, no test run, no launch.

**Verdict: APPROVE-WITH-FIXES.** M1–M3 must land before the OP4 connected gate
is run — the gate script this commit ships (`docs/research/2026-08-11-campaign-op-test-script.md`
§OP4 steps 3 and 13) contains steps that cannot pass against this code, so
running it now burns a user session on a known-failing check.

---

## 1. Group-C re-point: per-option old-consumer enumeration

Seven ids moved from the client-local `GameplaySettings` record (settings.json)
to `RuntimeCharacterOptionsState.GetOptionBit`. For each: every read site of the
OLD value at `22b86b9f^`, and whether it moved.

### AutoTarget (`0x0D`, Options1 `0x2000`, batched, ClientDefault **ON**)

| Old read site (`22b86b9f^`) | Moved? |
|---|---|
| `Composition/FrameRootComposition.cs:599` → `CombatCameraTargetSource(d.Settings, …)` | ✅ → `CharacterOptionCombatSettingsSource` |
| `Composition/SessionPlayerComposition.cs:770` → `LiveCombatAttackOperations(… d.Settings …)` | ✅ |
| `Composition/InteractionRetainedUiComposition.cs:286` → `LiveCombatTargetOperations(autoTarget: …)` | ✅ |
| `UI/Layout/CombatUiController.cs:116` (LED click writes) / `:193` (LED seed reads) | ❌ **NOT moved** |
| `Settings/RuntimeSettingsController.cs:236` `public bool AutoTarget => Gameplay.AutoTarget` | ❌ now dead (no consumer) |
| `Settings/RuntimeSettingsController.cs:422,433` (`SetCombatGameplay` VM mirrors) | ❌ still mirrors the orphan |
| `Headless/Hosting/HeadlessGameplayOperations.cs:127` `AutoTarget => true` (hardcoded) | ❌ never re-pointed |

### AutoRepeatAttack (`0x00`, Options1 `0x02`, **auto-save**, ClientDefault ON)

| Old read site | Moved? |
|---|---|
| `Composition/SessionPlayerComposition.cs:770` → `LiveCombatAttackOperations` (→ `RuntimeCombatAttackState:380,403,411`) | ✅ |
| `UI/Layout/CombatUiController.cs:114` / `:192` | ❌ **NOT moved** |
| `Settings/RuntimeSettingsController.cs:238` | ❌ now dead |
| `Headless/Hosting/HeadlessGameplayOperations.cs:126` `AutoRepeatAttack => false` | ❌ never re-pointed |

### ViewCombatTarget (`0x07`, Options1 `0x80`, batched, ClientDefault **OFF**)

| Old read site | Moved? |
|---|---|
| `Combat/CombatCameraTargetSource.cs:44` (via injected source) | ✅ |
| `UI/Layout/CombatUiController.cs:118` / `:194` | ❌ **NOT moved** |
| `Settings/RuntimeSettingsController.cs:240` | ❌ now dead |

⚠️ **Observable-default flip.** `GameplaySettings.Default.ViewCombatTarget = true`
(`src/AcDream.UI.Abstractions/Panels/Settings/GameplaySettings.cs:51`) but bit
`0x80` is CLEAR in retail's `CharacterOptions1.Default = 0x50C4A54A`
(→ `CharacterOptionTable.cs:118` `ClientDefault: false`). A fresh install's
combat camera stops tracking the target by default. No register row filed.

### VividTargetingIndicator (`0x0E`, Options1 `0x8000`, batched, ClientDefault ON)

| Old read site | Moved? |
|---|---|
| `Composition/InteractionRetainedUiComposition.cs:676` → `VividTargetRuntimeBindings` | ✅ (polled lambda) |
| `Panels/Settings/SettingsPanel.cs:322` | n/a — `SettingsPanel` has **no production construction site** (`new SettingsPanel(` appears only in `tests/AcDream.UI.Abstractions.Tests/…:40`) |

### CoordinatesOnRadar (`0x14`, Options1 `0x400000`, batched, ClientDefault ON)

| Old read site | Moved? |
|---|---|
| `Composition/LivePresentationComposition.cs:836` → radar snapshot | ✅ (polled lambda) |

### LockUI (`0x33`, Options2 `0x1000000`, **auto-save**, ClientDefault OFF)

| Old read site | Moved? |
|---|---|
| `Composition/LivePresentationComposition.cs:837` `uiLocked:` (radar) | ✅ polled — converges live |
| `Composition/InteractionRetainedUiComposition.cs:477` `host.Root.UiLocked = …` | ⚠️ moved but **one-shot at mount**, never re-read → does NOT converge |
| `Net/LiveSessionRuntimeFactory.cs:387` `ToggleUiLock` read | ✅ reads the bit — but see **M3** |
| `Settings/RuntimeSettingsController.cs:355,359,362,369` (`SetUiLocked` writer) | ❌ still writes the orphan, and is still called from `ToggleUiLock` |

Note: `LockUI` is deliberately NOT a Character-tab row
(`CharacterOptionsPageControllerTests.ExcludedIds_HaveNoRow`), and the OP4 gate
script's Group-C section (items 17–18) does not list it — so this re-point ships
ungated.

### AcceptLootPermits (`0x10`, Options1 `0x80000`, **auto-save**, ClientDefault OFF)

| Old read site | Moved? |
|---|---|
| `Net/LiveSessionRuntimeFactory.cs:452` (`ClientCommandController.Bindings.AcceptLootPermits`) | ✅ |
| `Net/LiveSessionRuntimeFactory.cs:454` (`SetAcceptLootPermits`) | ✅ (now sends 0x0005 **and** writes the orphan) |
| `UI/ClientCommandController.cs:444,450` (`/consent on|off`) | ✅ via the bindings above |
| `Net/LiveSessionCommandRouter.cs:458,459` | ✅ pure forwarders |

### ToggleRun (`0x0A`, Options1 `0x400`, batched, ClientDefault ON) — Group B

| Old read site | Moved? |
|---|---|
| `Input/DispatcherMovementInputSource.cs:77` hardcoded `!walking` | ✅ → `RunAsDefaultMovement != walking` |
| `Panels/Settings/SettingsPanel.cs:314` | n/a (test-only surface) |

---

## 2. MUST-FIX

### M1 — The Character tab is seeded before login and never re-seeded; Reset then writes the stale baseline over server truth

`RetailUiRuntime.Initialize()` (`src/AcDream.App/UI/RetailUiRuntime.cs:371`)
calls `MountOptionsPanel()`, which calls
`CharacterOptionsPageController.Bind` (`RetailUiRuntime.cs:2010`). That whole
chain runs inside `lease.Mount(…)`
(`src/AcDream.App/Composition/InteractionRetainedUiComposition.cs:876`), which
runs inside `InteractionRetainedUiCompositionPhase` — a **startup** phase of
`GameWindow.OnLoad` (`src/AcDream.App/Rendering/GameWindow.cs:1231`, chain at
`:1325-1369`). No network session exists yet.

So at `CharacterOptionsPageController.cs:368`:

```csharp
bool initial = bindings.CurrentValue(spec.Id);   // == GetOptionBit(...)
checkbox.Selected = initial;
var row_ = new BoolOptionRow(initial, entry.ClientDefault, apply: …);
```

`initial` is read from `RuntimeCharacterOptionsState`'s **constructor** words
(`RuntimeCharacterState.cs:671-672`, `DefaultOptions1 = 0x50C4A54A` /
`DefaultOptions2 = 0x00948700`), not the character's server options.
`PlayerDescription` installs the real words later at
`src/AcDream.Runtime/Session/LiveSessionEventRouter.cs:223` (`Options.Replace`),
and the only subscriber to `OnCharacterOptionsChanged` is
`SyncChatFromServerOptions` (`Net/LiveSessionRuntimeFactory.cs:323-324`).
**Nothing re-seeds the 50 rows.** Consequences:

1. Gate-script step 3 ("Confirm the checkboxes reflect your character's actual
   state … seeded from the live server bit at panel-mount time") is false for
   every option whose server value differs from the client constructor default.
2. Worse — the `BoolOptionRow` `_saved` baseline is also stale. Take a batched
   option the server has ON but whose constructor default is OFF (e.g.
   `SideBySideVitals`). Row seeds `_current = _saved = false`. User clicks it →
   `SetCurrentValue(true)` → `TrySetOption` sees the bit already true → no-op
   (correct). User changes their mind and clicks **Reset** (or just switches
   tab, which is `OnHidden() → Reset()`, `OptionPageModel.cs:261`) →
   `Changed == true` → `RestoreSavedValue()` → `_apply(false)` →
   `SetSingleCharacterOptionRuntimeCmd(id, false)` → **the server bit is turned
   OFF.** A visually-idempotent "toggle then cancel" mutates real character
   state in the wrong direction. This is the "contract asserting a mechanism
   that does not exist" class the C4 closeout handoff flags as binding.
3. `host.Root.UiLocked = d.Character.Options.GetOptionBit(LockUI)`
   (`InteractionRetainedUiComposition.cs:477-486`) has the same shape but is a
   one-shot assignment, so retained-window locking never converges to the
   server bit — while the radar's `uiLocked` lambda
   (`LivePresentationComposition.cs:843-844`) polls and does converge. Two
   consumers of one option, permanently able to disagree.

The fix shape is a re-seed hook on `OnCharacterOptionsChanged` (the seam CH3
already established for chat) that re-runs each row's
`SetSeed(current)` — i.e. rewrite `_current`, `_saved`, and `checkbox.Selected`
without invoking `_apply`. That verb does not exist on `BoolOptionRow` today.

### M2 — Two writable copies: the Combat panel's three LEDs still drive the orphaned `GameplaySettings`

`src/AcDream.App/UI/Layout/CombatUiController.cs:113-118` writes and `:192-194`
reads `GameplaySettings.AutoRepeatAttack / AutoTarget / ViewCombatTarget`,
wired in production at `InteractionRetainedUiComposition.cs:644-648`
(`() => d.Settings.Gameplay`, `d.Settings.SetCombatGameplay`) and mounted at
`RetailUiRuntime.cs:366` (`MountCombat()`). Nothing gameplay-facing reads that
record any more — every behavioral consumer now reads the server bit. Net
effect:

- The retail Combat window's **Repeat Attacks / Auto Target / Keep in View**
  checkboxes are now decorative: clicking them changes combat behavior not at
  all, and never reaches the wire.
- They and the Character tab's rows for the *same three retail
  `PlayerOption`s* read from different stores, so they will visibly disagree.
- `RuntimeSettingsController.SetCombatGameplay` (`:415-442`) keeps persisting
  the divergent copy to settings.json, so the disagreement survives restarts.

This is exactly the "two writable copies" failure mode the CH3 research warned
about, and it is the one old read site of the three combat options that the
re-point missed. The Combat panel's LEDs must route through
`SetSingleCharacterOptionRuntimeCmd` and seed from `GetOptionBit`, in this
slice or an immediately-following one.

### M3 — `ToggleUiLock`'s new dual write can silently drop the visual lock

`src/AcDream.App/Net/LiveSessionRuntimeFactory.cs:392-400`:

```csharp
bool locked = !_domain.Character.Options.GetOptionBit(CharacterOptionId.LockUI);
SendSingleCharacterOption((uint)CharacterOptionId.LockUI, locked);
_interaction.Settings.SetUiLocked(locked);
```

`RuntimeSettingsController.SetUiLocked` opens with
(`src/AcDream.App/Settings/RuntimeSettingsController.cs:355`):

```csharp
if (Gameplay.LockUI == locked && _uiLockConverged) return;
```

with `_uiLockConverged = true` at construction (`:185`). **Before** this commit
`locked` was computed as `!Gameplay.LockUI`, so the guard could never trip.
Now `locked` is derived from a *different* store, so whenever the persisted
`Gameplay.LockUI` already equals the newly-computed value the method
early-returns and `_runtimeTargets?.ApplyUiLock(locked)` (`:360`) never runs —
the wire bit flips, the radar (polled) follows, and `host.Root.UiLocked` does
not. Concretely: a user with `"lockUI": true` in settings.json (set by
`/lockui` in any pre-OP4 session) presses the lock toggle once after login and
nothing happens to their windows, while the server is told the UI is locked.

---

## 3. SHOULD-FIX

### S1 — `DisplayTimeStamps` only reaches a small subset of transcript lines

`RuntimeCommunicationState.AddText` (`:219-227`) is documented as "the single
chokepoint every producer of player-visible interface text must go through",
and retail's `ClientSystem::AddTextToScroll @0x00563C50` genuinely is that
chokepoint. In acdream it is not. These producers write `ChatLog` directly and
therefore never get a timestamp:

| Producer | Site |
|---|---|
| All heard speech (say / tell / ranged) | `Runtime/Session/LiveSessionEventRouter.cs:293` `Chat.OnLocalSpeech` |
| Emotes / soul emotes | `LiveSessionEventRouter.cs:319,321` |
| Player-kill messages | `LiveSessionEventRouter.cs:325` |
| Turbine channels (allegiance/general/trade/LFG/RP/society) | `LiveSessionEventRouter.cs:~535-556` via `RouteTurbineChat` |
| All combat text | `CombatChatTranslator` (`LiveSessionEventRouter.cs:306`) writes `social.Chat` directly |
| `@age` / `@index` / `@clist` / `@hslist` / allegiance-info replies | `Core.Net/GameEventWiring.cs:162,176,183,190,197` |
| App-side echoes | `Net/LiveSessionRuntimeFactory.cs:165,169,418` |
| Headless chat | `Headless/Hosting/HeadlessGameplayOperations.cs:242` |

Only `ServerMessage (0xF7E0)` and the WeenieError family route through `AddText`
(`LiveSessionEventRouter.cs:310-317`, `GameEventWiring.cs:~655-668`). The gate
script's own step 13 ("send/receive a new chat line … e.g. `/who` or any inbound
tell/say. The NEW line should be prefixed") is therefore unsatisfiable for the
tell/say half. Either move the remaining producers behind `AddText` or narrow
the register/gate wording to what is actually implemented.

Direction check (nothing leaks where it must not): the `ClientLocal`/SpewBox
branch returns before the prefix (`:213-217`) — correct and tested; there is no
chat-log-file writer in the tree (`grep` for `ChatLogFile`/`WriteChatLog` is
empty); `AddText` is display-only so nothing reaches the wire; the chat
filters in `Panels/Chat/ChatCommandRouter.cs` / `ChatInputParser.cs` match on
*outbound input*, not transcript lines, so a prefix cannot confuse them.

### S2 — `FogMode.Off` is not honored by the sky shader

`WeatherSystem` forces `fogMode = FogMode.Off` (`WeatherState.cs:326-327`) and
the mesh/terrain fragment shaders respect it
(`Rendering/Shaders/mesh_modern.frag:48-49` `if (mode == 0) return lit;`,
`terrain_modern.frag:160`). But `Rendering/Shaders/sky.vert:159-162` computes
`vFogFactor` from `uFogParams.x/.y` only and **never reads `uFogParams.w`**;
`sky.frag:62` then blends the dome toward the fog color. With Disable Distance
Fog ON the world is crisp to the horizon while the sky dome keeps its fog-color
band — a visible seam exactly where the option is supposed to remove one. This
is a visual-gate item (gate step 14) and needs the user's eyes either way, but
the shader asymmetry is a code fact, not a judgement call.

### S3 — Observable-default changes are not registered

The register hunk adds/updates `AP-194`, `TS-73`, `TS-75`–`TS-80`. None of them
records that seven options changed their *effective default source* from
`GameplaySettings.Default` (or the user's persisted settings.json) to the
retail constructor word, nor that `ViewCombatTarget`'s observable default flips
`true → false`, nor that `GameplaySettings.{AutoTarget, AutoRepeatAttack,
ViewCombatTarget, LockUI, VividTargetingIndicator, CoordinatesOnRadar,
AcceptLootPermits, ToggleRun}` are now orphaned-but-still-written. The campaign
contracted register rows for observable-default changes; these are the rows.

### S4 — The timestamp format is culture-dependent

`RuntimeCommunicationState.cs:225`:

```csharp
text = DateTime.Now.ToString("H:mm:ss ") + text;
```

No `IFormatProvider`, so `:` is `CurrentCulture.DateTimeFormat.TimeSeparator`.
Retail's CRT `strftime("%#H:%M:%S ")` emits literal colons unconditionally. On a
machine whose culture uses `.` (e.g. `fi-FI`) acdream renders `13.05.09 `.
The new test `AddText_TimestampsTrue_PrefixesTranscriptLine` pins
`^\d{1,2}:\d{2}:\d{2} …` and would fail on such a machine — i.e. the bug is
also a latent CI/locale flake. Use `CultureInfo.InvariantCulture`.

### S5 — The headless host's combat-option reads were not swept

`src/AcDream.Headless/Hosting/HeadlessGameplayOperations.cs:126-127` still
hardcodes `AutoRepeatAttack => false` and `AutoTarget => true`. OP7 gave
headless bots a `characterOptions` config that writes these very bits, so a bot
configured with `AutoRepeatAttack` now sets the server bit while its own combat
loop still reads `false`. Same interface (`ICombatGameplaySettingsSource`
shape), same options, opposite direction from the graphical host after this
commit.

### S6 — Test coverage gaps on the two changed *consumers*

The Runtime-level OFF paths are well covered (fog unbound/false/true, timestamps
unbound/false/true + SpewBox exemption, `RunAsDefaultMovement` unbound/true/false
live-poll). What is **not** covered:

- `DispatcherMovementInputSource.Capture()` — the file changed
  (`:83`) but `tests/AcDream.App.Tests/Input/DispatcherMovementInputSourceTests.cs`
  was not touched. The existing
  `CapturesDispatcherHeldStateAndRetailWalkModifier` only exercises
  option-unbound (⇒ `true`) + walk-held. Nothing pins the four-way truth table
  (option × modifier), nor the `|| AutoRunActive` override with the option OFF.
- The per-page scoped Apply/Reset/Defaults wiring —
  `OptionsPanelControllerTests.cs` was not touched, so the mechanism the commit
  message calls out (`UiElement.FindDescendant` scoping) has no test that a
  Chat-page Apply drives the Chat page's model rather than Character's.
- Nothing pins the login-before-seed transition (M1) — unsurprising, since the
  mechanism does not exist.

---

## 4. NOTE

- **N1 — `FindDescendant` promotion changed semantics, and the scoping is
  sound.** The old private `UiTabPanel.FindDescendant` skipped the root; the new
  `UiElement.FindDescendant` (`src/AcDream.App/UI/UiElement.cs:312-334`) matches
  the root first. `UiTabPanel:162,163,205,208` now calls the root-inclusive
  version — harmless here because the panel's own id (`0x1000018D`) is not a tab
  button or page id, but it is a real behavior change worth knowing.
  On the OP3 re-review's duplicate-id hazard: the committed fixture
  `options_panel_2100006E_1000018D.json` has 111 nodes / 38 unique ids and **16
  duplicated ids** (`0x100002CE/CF/D0` ×16 each; `0x100001FC/FD/FE` ×3). The
  scoped searches are genuinely scoped — each page slot is a direct child of the
  root and owns its own Apply/Reset/Defaults instance:
  `/0x1000018D/0x10000211/{0x100001FC,FD,FE}`,
  `/0x1000018D/0x10000213/{…}`, `/0x1000018D/0x1000050C/{…}`. A Character-page
  search structurally cannot reach a Chat-page button. Separately,
  `0x100001FA`/`0x100001FB` (the Character ListBox + scrollbar) are **unique**,
  so `CharacterOptionsPageController`'s flat `layout.FindElement` lookups
  (`:280`, `:293`) are unambiguous — but that is a property of the data, not of
  the code, and nothing asserts it.
- **N2 — dead surface.** `RuntimeSettingsController.AutoTarget` /
  `AutoRepeatAttack` / `ViewCombatTarget` (`:236-240`) now have zero consumers.
  `GameplaySettingsState` (`Combat/LiveCombatAttackOperations.cs:23`) survives
  only in `tests/AcDream.App.Tests/Combat/CombatCameraTargetSourceTests.cs:16`.
- **N3 — thread-safety is consistent; the binding fields are not volatile.**
  `GetOptionBit` (`RuntimeCharacterState.cs:858-865`) reads one word through the
  existing `Volatile.Read`-backed `Options1`/`Options2` properties, one static
  `Dictionary` lookup, no lock, no allocation (`CharacterOptionTableEntry` is a
  `readonly record struct`, `TryGetValue` uses `out`). That matches every other
  read on the type. The three new `Func<bool>?` source properties are plain auto
  properties written once in the `GameWindow` constructor
  (`Rendering/GameWindow.cs:649-678`) before the render loop and the network
  thread exist, so publication is fine in practice but is not formally
  guaranteed. `WeatherSystem` is a get-only property assigned once
  (`Runtime/World/RuntimeWorldEnvironmentState.cs:144,149`), so the fog binding
  cannot be lost on session reset. Per-frame cost is ~3 dictionary lookups
  (fog + radar ×2) + 1 in `Capture()` — negligible, and none of them is on a
  Slice-I zero-allocation resolve path.
- **N4 — `FindCheckbox` is direct-children-only.**
  `CharacterOptionsPageController.cs:385-392` searches the row root and its
  *direct* children while the commit simultaneously promotes a recursive
  `FindDescendant` for exactly this class of lookup. If a template ever nests
  the checkbox one level deeper, all 50 rows silently degrade to a log line.
- **N5 — the timestamp weakens `ChatLog`'s text-keyed dedupe.**
  `ChatLog.OnSystemMessage` (`src/AcDream.Core/Chat/ChatLog.cs:266-278`) dedupes
  on exact text within 1 s. Timestamped and untimestamped copies of the same
  logical line (one via `AddText`, one via a direct producer from S1) can no
  longer match; two identical lines straddling a second boundary also stop
  matching. This is an acdream-only workaround, not retail behavior, so the
  consequence is a re-appearing duplicate, not a correctness break.
- **N6 — autorun ignores the new default; retail's own seam does not.**
  `Run: (_movement.RunAsDefaultMovement != walking) || AutoRunActive`
  (`Input/DispatcherMovementInputSource.cs:83`). The `!= walking` half is
  byte-correct against retail: `CommandInterpreter::SetHoldRun @0x006b3370`
  computes `(!hold_run) != (!UITogglesRun())` — i.e. `holdRun XOR ToggleRun` —
  and passes that to `CMotionInterp::set_hold_run`, which is precisely where
  acdream's `MovementInput.Run` lands
  (`Runtime/Gameplay/PlayerMovementController.cs:1166-1168`). The wire encoding
  is untouched and stays retail-correct in both option states: `Run` →
  `IsRunning` → `HoldKey.Run`/`HoldKey.None` + `ForwardSpeed`
  (`Runtime/Gameplay/LocalPlayerOutboundController.cs:234-248`), so no
  double-inversion. There is also no collision with the `ToggleRunLock`
  keybind latch — that drives `_autoRunActive`, a separate field, and the
  property's doc comment says so explicitly. What is newly *reachable* is the
  `|| AutoRunActive` clause: `CommandInterpreter::SetAutoRun @0x006b4850` sets
  `auto_run` and calls `ApplyCurrentMovement()` — it does **not** force run —
  so with the option OFF an autorunning acdream player runs where retail would
  walk. That `||` predates this commit (2026-07-30 movement-parity audit) but
  was unobservable while the default was hardcoded.
- **N7 — citation accuracy.** `RuntimeLocalPlayerMovementState`'s doc comment
  names "retail `PlayerOption RunAsDefaultMovement`". No such member exists —
  `acclient.h:4165` spells id `0xA` `ToggleRun_PlayerOption`, and
  `RunAsDefaultMovement` is ACE's name for the bit. The binding
  (`GameWindow.cs:677`) and the row's `RetailName` (`"ToggleRun"`, which is what
  hashes `ID_PlayerOption_ToggleRun` — confirmed at pseudo-C `:171586`) are both
  correct; only the prose is wrong.
- **N8 — `Defaults` can emit a burst of 21 immediate `0x0005` sends.**
  `OptionPage.Defaults()` calls `RestoreDefaultValue()` on all 50 rows, each of
  which publishes `SetSingleCharacterOptionRuntimeCmd`. `TrySetOption` no-ops on
  unchanged values, so the worst case is bounded by the auto-save set (21 ids).
  Retail does the same thing; recorded here only so a future wire-rate reviewer
  does not treat it as a bug.