diff --git a/docs/research/2026-08-11-op4-review-mechanism.md b/docs/research/2026-08-11-op4-review-mechanism.md new file mode 100644 index 00000000..de76e1b4 --- /dev/null +++ b/docs/research/2026-08-11-op4-review-mechanism.md @@ -0,0 +1,545 @@ +# Campaign OP slice OP4 — MECHANISM-FAITHFULNESS review + +**Commit under review:** `22b86b9f` "Campaign OP slice OP4 — the Character tab" +**Lens:** mechanism faithfulness (retail behaviour vs. the port). The +regression/blast-radius lens is a separate document. +**Contract:** `docs/plans/2026-08-10-options-panel-campaign.md` §4 OP4 + §2 D3/D7. +**Oracles:** `docs/research/2026-08-10-character-options-map.md` §2/§7.1; +`docs/research/2026-08-10-options-panel-structure.md` §3/§6/§7/§10.4; +`docs/research/2026-08-10-set-character-options-wire.md`; +`docs/research/named-retail/acclient.h`; +`docs/research/named-retail/acclient_2013_pseudo_c.txt`. +**Mode:** read-only. No build, no test run, no client launch. The only file +written is this one. + +--- + +## Verdict + +**APPROVE-WITH-FIXES** — with one hard sequencing condition: + +> **The connected gate must NOT be scheduled until MUST-FIX 1 lands.** The +> Character tab's 50 LEDs are seeded once, at *window startup*, before any +> network session exists, from `RuntimeCharacterOptionsState`'s +> constructor default word — so the panel cannot show the logged-in +> character's real options. Test-script items 3, 6, 7 and 8 all assert the +> opposite and will fail on the first run. + +The *substance* of the slice is right and is the best-evidenced work in the +campaign so far. The 50-row binding is exact, the DEFAULTS CORRECTION is +independently verified and the plan's own §1 note was genuinely wrong, the +cross-page button-id collision is real and the scoped fix is correct, the +Group-B/C consumers are correctly anchored, and the register bookkeeping is +honest — including two corrections (TS-75, the U1 defaults source) where the +implementer refused to wire a plausible-but-wrong seam. Both MUST-FIX items +are contained, retail-anchored, and touch three files. + +| Class | Count | Items | +|---|---|---| +| MUST-FIX | 2 | MF-1 stale row seed (never reads live option word); MF-2 Apply/Reset enable-gating not wired | +| SHOULD-FIX | 3 | SF-1 timestamp culture + over-claimed "byte-verified"; SF-2 test gaps (widget click path, header hashes); SF-3 test-script false-failure in item 8 + the enable-gate items | +| NOTE | 5 | N-1 `FindCheckbox` ignores the retail id; N-2 `FindDescendant` semantics changed on promotion; N-3 sky dome still fogs; N-4 AP-194 anchor mislabel; N-5 no refresh on server-side option echo | + +--- + +## 1. What I verified as CORRECT (evidence first) + +These are recorded because several are load-bearing for later slices and +because a future reader must not "rediscover" them as bugs. + +### 1.1 The 50-row binding — 18 rows traced end-to-end + +`src/AcDream.App/UI/Layout/CharacterOptionsPageController.cs:101-186` +(`Groups`) reproduces `docs/research/2026-08-10-options-panel-structure.md` +§7's authored sequence **exactly**: group sizes 3 / 15 / 6 / 11 / 7 / 7+1, +row-for-row in authored order, six header keys in authored order. Every +`RetailName` matches `acclient.h:4162-4218`'s literal enumerator spelling +(I diffed all 49 against the header). + +I traced **18** rows (the brief asked for 15) row → `PlayerOption` id → +word/mask → auto-save flag → client default → wire route, against +`CharacterOptionTable` (`src/AcDream.Runtime/Gameplay/CharacterOptionTable.cs`) +and, independently, against the two byte-verified jump tables transcribed in +character-options-map.md §1.3 (`IsAutoSaveOption`, VA `0x0059A62C`) and §1.4 +(`GetDefaultOptionValue`, VA `0x005D2A5C`). Note both tables are *inverted* +(`0x00` → `jump_table[0]` → `return 1`); the table's columns decode +correctly under that inversion in all 18 cases. + +| Row | id | word · mask | auto-save | ClientDefault | wire route | +|---|---|---|---|---|---| +| Keep Combat Targets in View | 0x07 | O1 `0x80` | no (idx 7 = 01) | false | dirty → `0x01A1` | +| Salvage Multiple | 0x22 | O2 `0x80` | no | false | dirty | +| Use Main Pack as Default | 0x29 | O2 `0x4000` | no | false | dirty | +| Vivid Targeting Indicator | 0x0E | O1 `0x8000` | no | **true** (idx 0x0E = 00) | dirty | +| Display 3D Tooltips | 0x08 | O1 `0x100` | no | true | dirty | +| Show Coordinates By the Radar | 0x14 | O1 `0x400000` | no | true | dirty | +| Disable Distance Fog | 0x30 | O2 `0x200000` | no (idx 0x30 = 01) | false (past table end) | dirty | +| Show Your Helm | 0x2F | O2 `0x100000` | **yes** (idx 0x2F = 00) | false (past end) | `0x0005` | +| Show Your Cloak | 0x32 | O2 `0x800000` | **yes** | false (past end) | `0x0005` | +| Ignore Allegiance Requests | 0x01 | O1 `0x04` | yes | false | `0x0005` | +| Ignore Fellowship Requests | 0x02 | O1 `0x08` | yes | **true** | `0x0005` | +| Share Fellowship XP | 0x0F | O1 `0x40000` | yes | true | `0x0005` | +| Accept Corpse Looting Permissions | 0x10 | O1 `0x80000` | yes | false | `0x0005` | +| Automatically Repeat Attacks | 0x00 | O1 `0x02` | yes | true | `0x0005` | +| Listen to Society Chat | 0x2E | O2 `0x80000` | yes | false (past end) | `0x0005` | +| Display Timestamps | 0x21 | O2 `0x40` | no | false | dirty | +| Run as Default Movement | 0x0A | O1 `0x400` | no | true | dirty | +| Listen to PK death messages | 0x34 | O2 `0x02000000` | no (ACE-sourced) | false | dirty — D3 wire+store only, AP-193 | + +The click path is `checkbox.OnClick` (`:381`) → `BoolOptionRow.SetCurrentValue` +→ the row's `apply` closure (`:371-380`) → `Bindings.SetOption` → +`SetSingleCharacterOptionRuntimeCmd` (`RetailUiRuntime.cs:2029-2031`) → +`LiveSessionCommandRouter.cs:173-177` → `LiveSessionRuntimeFactory.cs:347-355` +`SendSingleCharacterOption` → `RuntimeCharacterOptionsState.TrySetOption` +(`RuntimeCharacterState.cs:806-842`), which writes the local bit first and then +*either* `sendAutoSave` (`0x0005`) *or* `MarkDirty()`. That is retail's +`CPlayerModule::OnChanged @0x0059A8E0` step order exactly. The batched half +flushes through `OptionPage.AfterApply` → `SaveCharacterOptionsRuntimeCmd` +(`RetailUiRuntime.cs:1987-1988`) → `TryFlush` → `SendSetCharacterOptions`. ✅ + +D3's 50th row is wire+store only: no consumer, `HearPkDeathMessages` appears +in no Group-B/C binding, and AP-193 carries the ACE-sourced mapping. ✅ + +The 50th row's label key is right: I computed +`compute_str_hash("ID_PlayerOption_HearPKDeaths")` with the exact algorithm at +`DatStringResolver.cs:68-82` and got **`0x0D16E9A3`**, matching structure doc +§7's byte-verified string id. ✅ + +### 1.2 The DEFAULTS CORRECTION is correct — the plan's §1 "U1 closed" note was wrong + +This is the highest-stakes claim in the commit and it holds. Read directly +from the pseudo-C rather than from the implementer's summary: + +`acclient_2013_pseudo_c.txt:147331` — +`UIOption_Checkbox::SetPlayerOption @0x00486e80`, guarded at `00486ea3` by +`if (this->m_propName == 0 && …)`, contains at `00486f2d`: + +``` +this->m_default = PlayerModule::GetDefaultOptionValue((__return_1 + 4), this->m_playerOption); +``` + +and `PlayerOptionPage::AddToggleOption(enum PlayerOption) @0x004F2A00` +(`:245616`) calls `SetPlayerOption` then `OptionPage::RegisterOption` for +*every* row on this tab. So the Character tab's `m_default` **is** +`GetDefaultOptionValue` — i.e. `CharacterOptionTable.ClientDefault`, the table +OP1 already ported. Structure doc §7's "biggest open unknown" is genuinely +closed by this. + +The plan's §1 note ("U1 closed: `UIOption::InqDefaultGameplayOptionProperty +@0x004ef8d0` … the Defaults button restores DAT-authored values") is +**mis-scoped, and the implementer's rejection of it is right**. The pseudo-C +has exactly one caller of `InqDefaultGameplayOptionProperty` — line `:145204`, +inside `UIOption_Slider::SetGameplayOptionProperty @0x00485030`, the +`m_propName`-bound path. The only three `SetGameplayOptionProperty` call sites +in the whole binary are `gmChatOptionsUI::InitOptions`' two sliders +(`0x10000080`, `0x10000081` at `:170766`, `:170797`) and its filter bitfields +(`0x1000007F` at `:170826`, `:170882`). So the claim that DAT collection +`0x78000001` holds only those three Chat-tab entries is *consistent with the +decomp by construction*, and — more importantly — it is **not load-bearing**: +every Character-tab row takes the `m_propName == 0` PlayerOption branch, so +the DBProperty mechanism cannot reach this tab regardless of what the +collection contains. Had the implementer followed the plan's note, the +Defaults button would have restored nothing at all (no PlayerOption-range keys +exist in that collection). + +AP-194's amendment (`docs/architecture/retail-divergence-register.md:186`) +states this truthfully: it confirms the directive (use the `ClientDefault` +column) was followed, cites the new independent trace, and names the +Chat/Config `m_propName` mechanism as the thing that does *not* apply here. ✅ + +### 1.3 The cross-page button-id collision is real — verified from the fixture + +Counted occurrences of each element id in the committed host fixture +`tests/AcDream.App.Tests/UI/Layout/fixtures/options_panel_2100006E_1000018D.json`: + +| Element | Occurrences | +|---|---| +| `0x100001FA` Character ListBox | **1** | +| `0x100001FB` Character scrollbar | **1** | +| `0x100001FC` Apply | **3** | +| `0x100001FD` Reset | **3** | +| `0x100001FE` Defaults | **3** | +| `0x10000200` Config ListBox | 1 | + +Three physical instances each of Apply/Reset/Defaults (Character, Chat, +Config — matching structure doc §3.1's "identical geometry AND identical +element ids", all base `0x21000040/0x100002CD`). A flat +`ImportedLayout.FindElement` would have wired all three pages' buttons to +whichever instance the flat index happened to hold. The per-page scoped +`UiElement.FindDescendant(pageRoot, id)` in +`OptionsPanelController.cs:214-236` is the correct fix and matches retail's +own `GetChildRecursive(, 0x100001FC)` scoping in +`PlayerOptionPage::OnOptionChanged @0x004F27D0`. The Character ListBox and +scrollbar ids are unique, so the flat lookups at +`CharacterOptionsPageController.cs:246,256` are safe. ✅ + +### 1.4 All six group-header keys hash to §7's byte-verified string ids + +I ran `DatStringResolver.ComputeHash`'s algorithm by hand over the six +`HeaderKey` literals: + +| Key | Computed | structure doc §7 | +|---|---|---| +| `ID_CharacterOption_UIBehavior_Section` | `0x06489B6E` | `0x06489B6E` ✅ | +| `ID_CharacterOption_UIDisplay_Section` | `0x0A9BC99E` | `0x0A9BC99E` ✅ | +| `ID_CharacterOption_Grouping_Section` | `0x0CBAAFAE` | `0x0CBAAFAE` ✅ | +| `ID_CharacterOption_OtherPlayers_Section` | `0x0872DFFE` | `0x0872DFFE` ✅ | +| `ID_CharacterOption_CharacterBehavior_Section` | `0x08674D5E` | `0x08674D5E` ✅ | +| `ID_CharacterOption_Chat_Section` | `0x0987FE8E` | `0x0987FE8E` ✅ | + +And all 49 `ID_PlayerOption_` / `_Help` label keys the controller +constructs at `:343,352` exist verbatim as globals in the retail binary +(grepped out of `acclient_2013_pseudo_c.txt`; the truncated 32-char variants +alongside them are the known BN preview artifact). **No invented user-visible +English anywhere** — every label, tooltip and header resolves by name against +table `0x23000003`, and an unresolved string degrades to *no text* +(`:328-333`, `:346-351`) rather than a fabricated caption. ✅ + +### 1.5 Separator cadence, template indices, and the LED click contract + +`Bind` emits header → rows → separator per group (`:264-277`), i.e. 6 headers +and 6 separators = structure doc §7's "5 interior separators + 1 trailing" +(§7's PK-death note independently confirms the trailing `AddSeperator` after +`HearSocietyChat`). Template indices 0/1/2 match §1.5's authored arrays, and +index 2 is literally the constant in `AddToggleOption`'s own +`AddItemFromTemplateList(m_pOptionBox, 2, nullptr)` at `:245616`. ✅ + +The LED click contract works because the toggle template's checkbox +`0x10000219` authors DAT property `0x0B` = true (fixture +`options_2100002B.json:406,428-431` → `UiButton.ToggleBehavior`), so +`UiButton` self-flips `_selected` at MouseUp (`UiButton.cs:486`) before the +`Click` event invokes `OnClick`, and `checkbox.OnClick = () => +row_.SetCurrentValue(checkbox.Selected)` (`:381`) therefore reads the *new* +value. ✅ (See SF-2 — nothing pins this.) + +### 1.6 Consumers + +- **Display Timestamps.** Prefixed at + `RuntimeCommunicationState.AddText` (`:224-225`) — retail's + `ClientSystem::AddTextToScroll @0x00563C50` chokepoint — *after* the + both-ends trim and *after* the `ClientLocal (0x1A)` early return, which is + exactly retail's order (the CH2 doc comment on the same method already + records that retail "skips the timestamp prefix … for this type"). Format + `"H:mm:ss "` is the correct .NET spelling of `%#H:%M:%S ` (non-padded 24h + hour, padded minute/second, trailing space). Prefix applies only when the + bit is on and only to *new* lines. ✅ (See SF-1 for two caveats.) +- **Disable Distance Fog.** `WeatherSystem.Snapshot` forces + `FogMode.Off` (`WeatherState.cs:326-327`), `FogMode.Off == 0` + (`SkyState.cs:15`), the UBO carries it as `FogParams.w` + (`SceneLightingUbo.cs:130`), and both world shaders gate on it — + `mesh_modern.frag:48-49` and `terrain_modern.frag:160-161` both + `if (mode == 0) return lit;`. The shader-gate claim is real. ✅ (See N-3.) +- **Run as Default Movement.** `DispatcherMovementInputSource.cs:84` + computes `Run: (_movement.RunAsDefaultMovement != walking) || AutoRunActive`. + This is byte-correct *and better-evidenced than the commit claims*: retail's + `CommandInterpreter::SetHoldRun @0x006B3370` (`:699110-699118`) computes + `ecx = (hold_run == 0); edx_3 = (UITogglesRun() == 0); eax_3 = (ecx != edx_3)` + — an XOR of the modifier against the option, which is precisely the port's + `!=`. `ACCmdInterp::UITogglesRun @0x0058B180` (`:405377`) is + `PlayerModule::ToggleRun`, i.e. option `0x0A`, which is the id GameWindow + binds (`GameWindow.cs:676-678`). With the option ON the expression collapses + to the pre-OP4 `!walking`, so the default path is bit-identical. ✅ +- **Group C re-point.** `CharacterOptionCombatSettingsSource` + (`LiveCombatAttackOperations.cs:31-63`) reads `GetOptionBit` live for + AutoTarget/AutoRepeatAttack/ViewCombatTarget and is substituted at both + construction sites (`SessionPlayerComposition.cs:770`, + `FrameRootComposition.cs:599-603`). `VividTargetingIndicator`, + `CoordinatesOnRadar`, `LockUI` and `AcceptLootPermits` likewise + (`InteractionRetainedUiComposition.cs:286-288,485-487,687-694`, + `LivePresentationComposition.cs:840-847`). Toggling writes through + `TrySetOption` (local bit first, then wire) per CH3 — verified for the two + non-panel entrances too: `ToggleUiLock` and `SetAcceptLootPermits` now both + call `SendSingleCharacterOption` before the legacy settings mirror + (`LiveSessionRuntimeFactory.cs:394-401,476-481`), which closes the two + divergences character-options-map.md §0 named. ✅ + +### 1.7 Register bookkeeping + +TS-73's narrowing is **truthful and appropriately scoped** — it explicitly +says neither closed case lives inside `TrySetOption`'s own switch, so the +row's literal title claim stays accurate while the user-observable symptom is +recorded as fixed for `0x07`/`0x30` only. + +TS-75's `ForcedDayGroupIndex` correction is **verified**: +`RuntimeWorldDayGroupDefinition(Name, ChanceOfOccur, SkyObjectCount, Sky)` +(`src/AcDream.Runtime/World/RuntimeWorldEnvironmentState.cs:5-14`) is a +probability-weighted *weather-variety* group from Region `0x13000000`; the +field's only consumers are `RuntimeOptions.cs:55,122` / +`GameWindow.cs:639` env-var plumbing. Wiring `PersistentAtDay` to it would +have silently changed the character's weather odds while leaving night +falling — the row is right to call that "an incorrect fix masquerading as a +correct one". TS-76..TS-80 are each honest no-consumer rows with the correct +retail anchors; TS-80 correctly identifies `FellowshipShareXP` as the one +client-sourced option whose *packet field*, not the stored bit, is what ACE +reads. Counts are consistent. ✅ + +--- + +## 2. MUST-FIX + +### MF-1 — The 50 rows are seeded once at *window startup*, before login, and never re-read the live option word + +**The panel cannot display the logged-in character's options.** + +Chain of evidence: + +1. `CharacterOptionsPageController.Bind` reads each row's initial value once, + at build time: `bool initial = bindings.CurrentValue(spec.Id);` → + `checkbox.Selected = initial;` → `new BoolOptionRow(initial, …)` + (`CharacterOptionsPageController.cs:368-380`). Nothing re-reads it after + that. +2. `Bind` is called from `RetailUiRuntime.MountOptionsPanel` + (`RetailUiRuntime.cs:2010`), which runs inside `Initialize()` + (`:371`) — i.e. once, when the retained UI is composed. +3. The retained UI is composed by the **startup** graph: + `GameWindow.OnLoad()` (`GameWindow.cs:1231`) → + `GameWindowCompositionPipeline.Run(… InteractionRetainedUiCompositionPhase …)` + (`:1326`). This runs at window load, before any `WorldSession` exists. +4. Pre-login, `RuntimeCharacterOptionsState` holds its constructor defaults — + `_options1 = DefaultOptions1 (0x50C4A54A)`, `_options2 = DefaultOptions2 + (0x00948700)` (`RuntimeCharacterState.cs:645-647, 671-672`). The class even + carries a `_hasServerSeed` flag (`:676`) that the panel never consults. +5. So every LED shows ACE's *character-creation* default word — which is + neither the character's real state nor even retail's own Defaults column + (AP-194: the two disagree on `0x2D`/`0x2F`/`0x32`). + +Consequences on the connected gate as scripted: + +- Item 3 ("confirm the checkboxes reflect your character's actual state … + should show checked on open") — fails for every option the character has + ever changed. +- Item 6 ("relog … rows should still read whatever you left them at (server + echo)") — fails; the rows never see the echo. +- Items 7/8 (batched persist-vs-revert, judged by re-reading the row) — the + row is not a valid readout of persisted state. + +There is no wire corruption (`TrySetOption` no-ops on an unchanged bit, +`RuntimeCharacterState.cs:812-814`, and `Reset` only touches `Changed` rows), +and the *consumers* are all correct because they poll `GetOptionBit` live. +The damage is confined to the panel's own display and baseline — but that is +the entire deliverable of this slice. + +**Retail's own mechanism already fixes this, and the OP3 mechanism review +predicted it.** `docs/research/2026-08-11-op3-review-mechanism.md:424-427` +(note N2) reads: *"`SaveCurrentValue` re-reads the widget in retail. +`0x004868E5`: `m_current = vtable->GetValue(); m_saved = m_current;`. +`BoolOptionRow.SaveCurrentValue` commits its own `_current`. **Equivalent only +while every mutation goes through `SetCurrentValue`.***" OP4 is the slice that +violates that precondition — the option word has many other writers (the +PlayerDescription seed, `/consent`, `@join`, `ToggleUiLock`, headless +commands, the fellowship mutual-exclusion recursion inside `TrySetOption` +itself). + +And `UIOption_Checkbox::GetValue @0x00486f60` is not a widget-local read: +character-options-map.md §1.2 establishes it branches +`m_playerOption != Invalid → PlayerModule::GetOption`, i.e. **the live +server-synced option word**. Since `OptionPage.OnShown() => Apply()` +(`OptionPageModel.cs:256`) runs on the initial default-tab activation, on +every tab switch in, and on every window (re)open, porting `SaveCurrentValue` +faithfully makes the panel self-correct on exactly retail's schedule. + +**Fix.** Give `BoolOptionRow` the `GetValue` reader it is missing and use it +in `SaveCurrentValue`: + +- add a `Func? read` ctor parameter (OP4 already has the exact reader: + `bindings.CurrentValue(spec.Id)`); +- `SaveCurrentValue()` becomes `_current = _read?.Invoke() ?? _current; + _saved = _current;` — retail `@0x004868E0` verbatim; +- push the re-read value onto the widget (retail's `Refresh()`), i.e. the + row's existing `apply`-side `checkbox.Selected = value` needs an + equivalent on the save path, or `BoolOptionRow` gains a `refresh` callback. + Do **not** route the re-read through `apply` — that would send the value + back on the wire. + +Pin it with a test that changes the binding's underlying value *behind* the +row and asserts the row and the checkbox both converge on the next +`OnShown()`. Add a second test for the pre-login case (bind with the +constructor default word, seed the state, show the page, assert all 50 rows +match). + +### MF-2 — Apply/Reset enable-gating is never wired in production + +Structure doc §3.4 (`PlayerOptionPage::OnOptionChanged @0x004F27D0`): + +``` +eax = GetChildRecursive(, 0x100001FC); // Apply +eax_1 = GetChildRecursive(, 0x100001FD); // Reset +if (OptionPage::Changed(...) == 0) { SetState(0x0D) on both } // disabled +else { SetState(0x01) on both } // enabled +``` + +plus "`PostInit` calls `InitOptions()` then `OnOptionChanged(0)` so the +buttons start disabled", and "**Defaults (`0x100001FE`) is never +enable-gated**". §10.4's *"Behaviours that MUST be ported, not improved"* +lists this as item 3. + +`OptionPage.OnOptionChanged` exists (`OptionPageModel.cs:204`), fires as the +last statement of `Apply`/`Reset`/`Defaults` (`:227,240,250`) and on every +live edit via `AttachPageNotify` (`:211`) — but **nothing in `src/` ever +assigns it**. Grep across `src/` and `tests/`: the only assignments are in +`OptionPageModelTests.cs`. OP4 is the slice that wired the three buttons +(`OptionsPanelController.cs:214-236`) and owns the first page that has them; +`OptionPageModel.cs:201-202` says in so many words "OP4-6 bind buttons to this +seam". + +Everything needed is already present: `UiButtonStateMachine.Ghosted == 13` +(= retail `0x0D`) and `Normal == 1` (= `0x01`) +(`UiButtonStateMachine.cs:17,23`), and `UiButton.TrySetRetailState` +(`UiButton.cs:209-223`) already implements `Ghosted → Enabled = false` and the +inverse. `UiButton.OnEvent` already refuses clicks when `!Enabled` +(`:494`). + +**Fix.** In `OptionsPanelController.Bind`'s per-page loop, capture the +resolved Apply and Reset `UiButton`s and set +`page.OnOptionChanged = () => { apply.TrySetRetailState(page.Changed ? +UiButtonStateMachine.Normal : UiButtonStateMachine.Ghosted); reset.Try…; }`, +then invoke it once at bind time so the pair starts disabled (retail's +`PostInit` tail). Leave Defaults untouched. Pin: (a) both start disabled on a +freshly-bound page, (b) one LED click enables both, (c) Apply disables both, +(d) Defaults leaves them enabled when it actually changed something and +(e) the Defaults button is never disabled. + +Without this the connected gate silently accepts a missing MUST-PORT +behaviour — and the script's own "what to report" bullet ("Any Reset/Defaults +button that stays permanently disabled") reads as though the author believed +the gating shipped. + +--- + +## 3. SHOULD-FIX + +### SF-1 — Timestamp: culture-dependent separator, an over-claimed "byte-verified", and the ignored per-character format string + +Three separate problems in one two-line consumer +(`RuntimeCommunicationState.cs:224-225`). + +1. **Culture.** `DateTime.Now.ToString("H:mm:ss ")` uses the current culture, + and `:` in a .NET custom date/time format string is the *time-separator + placeholder*, replaced by `DateTimeFormatInfo.TimeSeparator`. On a culture + that uses `.` (fi-FI and friends) both production output and the test's + `Assert.Matches(@"^\d{1,2}:\d{2}:\d{2} …")` + (`tests/AcDream.Runtime.Tests/Gameplay/RuntimeCommunicationStateTests.cs`, + the OP4 block) change. Retail's CRT `strftime` emits a literal `:`. Use + `CultureInfo.InvariantCulture` (or escape as `H\:mm\:ss `) and make the + test culture-explicit. +2. **The "byte-verified" claim is wrong.** The doc comment at + `RuntimeCommunicationState.cs:100-104` calls `"%#H:%M:%S "` "the + byte-verified CRT strftime string". The wire oracle says the opposite: + `docs/research/2026-08-10-set-character-options-wire.md:941` open-unknown + **U6** — *"The default timestamp format string `"%#H:%M:%S "` is + BN-sourced, not byte-verified"* (and `:314-316` repeats it). The campaign's + §8 process rule is that decomp claims are byte-verified against the + PDB-paired binary; this one asserts verification the source explicitly + withholds. Either verify it (the operand decode recipe is in structure + doc's appendix) or downgrade the comment to "BN-sourced (wire doc U6)". + Note U6's own "only matters when the Chat tab implements timestamp + formatting" caveat is now stale — OP4 shipped the consumer. +3. **The per-character format is ignored.** Retail reads the format from + `GenericQualitiesData::InqString(m_pPlayerOptionsData, 1, &m_TimeStampFormat)` + (wire doc `:311-313`), and acdream's parser reads-and-discards it + (`:647`, "timestamp string (`0x80`) | read, discarded | ❌ | never sent"). + Hardcoding the constructor default is defensible, but it is a divergence + with no register row. File one (or fold it into TS-77's neighbourhood). + +### SF-2 — Two conformance gaps in an otherwise strong test suite + +The 52 tests are good — both-directions row conformance, the independently +transcribed authored order, the `ClientDefault` pin, the tab-hide revert +against real fixtures. Two things a mechanism reviewer would want pinned: + +- **The widget click path is never exercised.** + `ClickingARow_PublishesSetOption_WithTheAuthoredId` + (`CharacterOptionsPageControllerTests.cs`, "Behavioral" section) calls + `boolRow.SetCurrentValue(true)` directly. Nothing drives + `checkbox.OnClick`, so nothing pins the `ToggleBehavior` dependency the + whole LED interaction rests on (§1.5 above). If DAT property `0x0B` on + `0x10000219` ever fails to import, all 50 rows go inert and the suite stays + green. Add one test that dispatches MouseDown/MouseUp/Click at the built + checkbox and asserts both `Selected` and the published `SetOption`. +- **Only one of the seven string keys is hash-pinned.** + `HearPkDeathMessages_RetailNameHash_MatchesByteVerifiedStringId` pins + `0x0D16E9A3`. The six header keys are not pinned, even though structure doc + §7 gives all six byte-verified string ids. I verified all six by hand (§1.4) + — they are correct today — but a typo in a `HeaderKey` literal would + produce a silently blank header that only the user's eye catches. A + six-row `[Theory]` is three minutes of work. + +### SF-3 — Test-script items that can generate a false failure, and items that test nothing + +- **Item 8** ("Toggle a batched row WITHOUT clicking Apply, then relog. The + row should revert to its PRE-toggle value") is only true if the page is + hidden first. Structure doc §3.5: `CPlayerSystem::LogOffCharacter + @0x00563520` calls `SaveToServer(pm, 0)` — logout flushes the dirty blob. + In acdream the same holds (`GameRuntime.cs:333`, the shared flush body). A + batched toggle left un-reverted therefore *does* persist. It happens to + revert in the common path because reaching Exit Game requires switching to + the Gameplay tab (→ `OnHidden` → `Reset`), but a user who closes the client + another way will see the opposite of what the script promises. Add "close + the Options panel (F11) first" as an explicit step, or state both outcomes. +- **Items 10 and the "what to report" enable/disable bullets** ask the user + to judge Apply/Reset enable-gating that does not exist (MF-2). Once MF-2 + lands, add an explicit step: "on first open of the Character tab, Apply and + Reset are greyed out; click any LED and they light up; click Apply and they + grey out again; Defaults is never greyed out." +- **Item 3** as written is a good check and will *correctly* fail today — + that is the MF-1 detector. Keep it exactly as-is. + +Otherwise the section is thorough, states retail-expected outcomes per step, +and its "Explicitly NOT in scope" block correctly pre-empts the OP3 R1 lesson +for the nine no-consumer rows and the two weather/daylight rows. + +--- + +## 4. NOTE + +- **N-1 — `FindCheckbox` ignores the retail element id.** + `CharacterOptionsPageController.cs:384-391` returns the first `UiButton` + among the row root's direct children. Retail is + `GetChildRecursive(row, 0x10000219)` (`:245616`). Correct for today's + single-child template, but the commit *just promoted* + `UiElement.FindDescendant` for exactly this purpose — use it with the + retail id and delete the bespoke helper. +- **N-2 — `FindDescendant`'s semantics changed during promotion.** + `UiTabPanel`'s private version skipped the root; `UiElement.FindDescendant` + (`UiElement.cs:325-337`) matches the root itself first. `UiTabPanel` calls + it as `FindDescendant(this, entry.ButtonElementId/PageElementId)` + (`UiTabPanel.cs:162-163,205-208`), so a tab host whose own id collided with + a button/page id would now resolve to itself. No collision exists today + (host `0x10000208` vs buttons `0x1000020D…` and pages `0x10000211…`), but + the behaviour change rode along uncommented in a commit whose message + describes the promotion as a pure move. +- **N-3 — the sky dome still fogs when Disable Distance Fog is on.** + `sky.vert:159-162` computes `vFogFactor` from `uFogParams.xy` with no + `mode == 0` guard, unlike the two world shaders. So distant terrain stops + fading but the dome's horizon band still blends to fog colour. Retail's + `LScape::m_fFogEnabled = !value` may or may not reach its sky path — not + established either way. TS-73's "0x30 DisableDistanceFog … now behave + correctly" is therefore slightly overstated; the gate script's item 14 + wisely limits the expectation to "distant terrain/objects", so this is not + a false-failure risk, only a register-precision one. +- **N-4 — AP-194's anchor column mislabels the symbol.** The prose correctly + says `UIOption_Checkbox::SetPlayerOption @0x00486e80`; the "Retail anchor" + column at the end of the row says `PlayerModule::SetPlayerOption + @0x00486e80`. Same address, wrong class — worth fixing in a register that + exists to be grepped by symbol name. +- **N-5 — no refresh on the server's own option echo.** Even after MF-1's + `GetValue` re-read lands, a change made *while the Character tab is + visible* (typing `/consent on`, a mid-session PlayerDescription, a plugin) + will not repaint the LED until the next hide/show. Retail has + `CM_UI::SendNotice_PlayerOptionChanged` / `SendNotice_RefreshOptionsPanel` + (structure doc §3.7) and acdream already has the hook — + `LiveSessionRuntimeFactory`'s `OnCharacterOptionsChanged` + (character-options-map.md §4.1). Wiring it is small; if it is deferred, + file a register row rather than leaving it implicit. + +--- + +## 5. Fix-order recommendation + +1. MF-1 (`BoolOptionRow` `GetValue` re-read + widget refresh) — blocks the gate. +2. MF-2 (enable-gating) — blocks the gate's own acceptance criteria. +3. SF-3 (script items 8/10 + the new enable-gate step) — do this *with* MF-2 + so the script and the code land consistent. +4. SF-1, SF-2 — same round, cheap. +5. N-1..N-5 — coordinator's call; N-4 is a one-word register edit and N-5 + wants either a wire-up or a row before OP9's reconciliation.