acdream/docs/research/2026-08-11-op6-review.md
Erik e318e8628d docs: OP6 review — REJECT (byte-decoded): zero-fold captions, inverted audio toggles, one-of-five fonts
M1: the 'U4 resolved: zero range captions' claim is the SAME BN zero-fold
artifact lane A caught on AddHeader — the bytes at 0x0049E4C6 load two
string-id globals (ID_Graphics_Value_Hard/_Soft) and all six labelled
sliders decode cleanly (Stiffness Soft/Hard, Adjustment Slow/Fast, FOV
Narrow/Wide, Brightness Dark/Bright, Performance Speed/Detail, Degrade
Close/Far) — U4 closes the OPPOSITE way; OP5's Chat sliders already
ported the same mechanism. M2: the three Sound 'Disabled' toggles are
sense-INVERTED (the INI key names the string; the registered static is
effect_sounds_enabled = 1) — SFX/ambient ship muted for every user
including existing settings.json installs, undoing Campaign A; a test,
AP-199's wording, and gate step 6 all enshrine it. M3: UI_ChatFontFace
ships 1 of retail's 5 authored choices (all five font strings verbatim
in the binary) with default index 2 out of range — a self-inflicted
copy of the genuine LandscapeDrawDistance oddity. S1: the sixth
trailing separator missing (retail builds 39 items); lane A's '24 rows'
is itself stale (27 is right). S2: Screen Brightness overloads Gamma
([-1,1] default 0 into a [0.5,2.0] multiplier defaulting 1.0). S3:
tooltips reach 12/30 rows (TooltipText exists only on UiButton). S4:
the 800x600 default is absent from the hardcoded resolution list. S5:
the tests pin safe seams and miss every risky one (no ApplyAudio value
assertion, no label/choice conformance, no per-row default pin, no
count pin). A large verified-clean catalog is recorded so the rework
does not re-litigate.

Rework round 1 queues behind the OP5 fixer.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-08-11 07:35:32 +02:00

28 KiB
Raw Blame History

OP6 (Config tab) — combined dual-lens review

Commit under review: f5ac1742 "feat(ui): Campaign OP slice OP6 — the Config tab" Contract: docs/plans/2026-08-10-options-panel-campaign.md §4 OP6 + §2 D1 Research base: 2026-08-10-options-panel-structure.md §4/§9; 2026-08-10-keyboard-config-and-gameplay-tab.md §7.3 Reviewer: combined mechanism × blast lens, Opus, 2026-08-11. Read-only.

Verdict: REJECT

Three MUST-FIX findings, two of them contradicting decompiler claims the slice asserts as resolved fact in code comments, the commit message, AND the connected-gate script. One of the three (M2) mutes sound effects and ambient audio for every profile — fresh and existing — on the next launch, which is a direct regression of Campaign A (closed 2026-08-08). Because the gate script instructs the user to treat both wrong behaviours as expected (§OP6 steps 6 and "What to report"), a connected gate as written would certify the defects rather than catch them. That combination — false primary-source claims + a user-visible regression + an acceptance script that blesses them — is past APPROVE-WITH-FIXES.

Everything else in the slice is unusually good: the template-index mapping, all eight authored template ids, every real-unit slider range, every truncated label key, the AddSliderOption BN-artifact decode, and the scoped-scrollbar fix all verify clean against the binary and the committed fixture (see §"Verified correct" below). The failures are concentrated and fixable.


Evidence discipline for this review

check_exe_pdb.py C:\Users\erikn\Downloads\acclient.exeMATCH (linker 2013-09-06T00:17:56Z, CodeView GUID 9e847e2f-777c-4bd9-886c-22256bb87f32). All byte quotes below were decoded from that binary via a PE VA→file-offset reader; image base 0x400000, .text VA 0x401000 / raw 0x1000.


MUST-FIX

M1 — the "U4 resolved: retail ships ZERO range captions" claim is FALSE (byte-disproved)

Claim under review (src/AcDream.App/UI/Layout/ConfigOptionsPageController.cs:44-58):

U4 resolved: retail ships ZERO slider range captions on this tab. Every one of the seven PlayerOptionPage::SetSliderLabel calls this tab makes passes literal (0, 0) … This controller therefore never touches the range-caption text children (0x1000021E/0x1000021F) at all — omitting them is the faithful port, not a shortcut.

Repeated in the commit message ("retail ships ZERO range captions on this tab (every SetSliderLabel call passes literal string id 0)") and in the gate script (docs/research/2026-08-11-campaign-op-test-script.md:535, and :653 which tells the user to report a caption as a defect).

The claim rests entirely on the Binary Ninja pseudo-C rendering PlayerOptionPage::SetSliderLabel(this, eax_5, 0, 0). That rendering is the same zero-fold artifact the lane-A researcher already caught and worked around: in the identical function BN also renders PlayerOptionPage::AddHeader(this, 0) six times, yet lane A byte-decoded six different header string-id globals (2026-08-10-options-panel-structure.md:492-499) — and this slice itself uses those decoded header keys. The implementer applied the correct skepticism to AddHeader and not to SetSliderLabel, in the same 60 lines of pseudo-C.

Byte evidence — the operands are globals, not immediates. Camera Stiffness, 0x0049E4C4:

0049E4B8  6a 01                    push 1                       ; arg3 = 1 (labelled)
0049E4BA  68 a0 e5 83 00           push offset Camera_Stiffness
0049E4BF  8b ce                    mov  ecx, esi
0049E4C1  e8 a7 45 05 00           call PlayerOptionPage::AddSliderOption
0049E4C6  8b 0d 94 e7 83 00        mov  ecx, [0083E794]         ; <- arg4, NOT 0
0049E4CC  8b 15 90 e7 83 00        mov  edx, [0083E790]         ; <- arg3, NOT 0
0049E4D2  51 52 50                 push ecx / push edx / push eax
0049E4DB  8b ce                    mov  ecx, esi
0049E4E0  e8 9b 46 05 00           call PlayerOptionPage::SetSliderLabel

The PDB names those globals (acclient_2013_pseudo_c.txt:1148623-1148634):

VA Symbol
0x0083E768 / 0x0083E76C ID_Graphics_Value_Dark / _Bright
0x0083E770 / 0x0083E774 ID_Graphics_Value_Speed / _Detail
0x0083E778 / 0x0083E77C ID_Graphics_Value_Close / _Far
0x0083E780 / 0x0083E784 ID_Graphics_Value_Narrow / _Wide
0x0083E788 / 0x0083E78C ID_Graphics_Value_Slow / _Fast
0x0083E790 / 0x0083E794 ID_Graphics_Value_Soft / _Hard

They are uint32_t const … = 0x0 in .data (runtime-filled by string-table registration) — which is exactly why BN constant-folded them to 0.

All six call sites decoded, giving the complete U4 answer:

Slider SetSliderLabel VA arg3 (low, child 0x1000021E) arg4 (high, child 0x1000021F)
Camera_Stiffness 0x0049E4E0 ID_Graphics_Value_Soft ID_Graphics_Value_Hard
Camera_AdjustmentSpeed 0x0049E51D ..._Slow ..._Fast
Render_FieldOfView 0x0049E556 ..._Narrow ..._Wide
Render_ScreenBrightness 0x0049E614 ..._Dark ..._Bright
Render_GraphicsPerformance 0x0049E65D ..._Speed ..._Detail
Render_DegradeDistance 0x0049E68E ..._Close ..._Far

Six labelled sliders ↔ six pairs ↔ lane A's six observed pairs, in declaration order, each pairing semantically obvious. Input_MouseLookSensitivity (arg3=0, template idx3) correctly gets no SetSliderLabel — and idx3 has no caption children. U4 is now closed in the opposite direction from the commit's claim.

Cross-check that removes all doubt: OP5 already ported this exact mechanism. gmChatOptionsUI::InitOptions renders as SetSliderLabel(this, esi, 0, 0) (acclient_2013_pseudo_c.txt:170779), yet at 0x0049FD37 the bytes are a1 2c e8 83 00 / 8b 0d 28 e8 83 00 / 50 51 56[0x0083E82C] / [0x0083E828], i.e. ID_UI_Value_Opaque / ID_UI_Value_Transparent — and ChatOptionsPageController.cs:313-315 renders them. The campaign's own gate script contradicts itself between line 381 (Chat: captions present) and line 535 (Config: "retail ships zero range captions").

Blast: the authored idx6 template has both caption children (Fixtures/options_2100002B.json, element 0x1000021D → children 0x1000021B, 0x1000021C, 0x1000021E, 0x1000021F), so six rows currently render two empty caption slots each. The fix is a straight reuse of ChatOptionsPageController.SetRangeLabel's shape at ConfigOptionsPageController.cs:681-729 (pass the two keys per row), plus correcting the class doc, the commit-message claim in the ledger, and gate-script lines 535 and 653.


M2 — the three Sound "Disabled" toggles are semantically inverted; SFX + ambient ship MUTED

What the code does. AudioSettings gained SfxDisabled = true / AmbientDisabled = true / InterfaceDisabled = true (src/AcDream.UI.Abstractions/Panels/Settings/AudioSettings.cs:51-55), and the engine write became (src/AcDream.App/Settings/RuntimeSettingsTargets.cs:147-148):

engine.SfxVolume     = audio.SfxDisabled     ? 0f : audio.Sfx;
engine.AmbientVolume = audio.AmbientDisabled ? 0f : audio.Ambient;

What retail actually stores. gmConfigUI::InitOptions @0x0049E435 is SetDefaultValue(1, 0x3f800000); the implementer read the 1 as "Disabled = true". But Sound_SoundDisabled is only the INI key string ("Sound.SoundDisabled", acclient_2013_pseudo_c.txt:718239). The preference is registered onto an enabled-sense variable (SoundManager::InitPrefs @0x005503F0, :346727):

UserPreferences::RegisterPreference(&SoundManager::effect_sounds_enabled,  &Sound_SoundDisabled, );
UserPreferences::RegisterPreference(&SoundManager::ambient_sounds_enabled, &Sound_AmbientSoundDisabled, );

and every read is enabled-sense (:346813, :346838, :347014, :347041):

if (SoundManager::effect_sounds_enabled != 0 && (s_bPlaySoundOnlyWhenActive == 0 || Device::m_bIsActiveApp != 0))

with .data statics SoundManager::effect_sounds_enabled = 0x1, ambient_sounds_enabled = 0x1, interface_sounds_enabled = 0x1 (:1102289, :1102294, :1102299), and SoundManager::Init forcing effect_sounds_enabled = 0 only when SoundOK() fails (:346769). Retail's SetDefaultValue(1) means sound ENABLED. The slice mapped it to Disabled = true, then wired that to a hard zero.

(Contrast Sound_PlaySoundOnlyWhenActive: positive-sense name, static = 0x1 at :1102304, and the implementer's PlaySoundOnlyWhenActive = true is correct — which shows the three "Disabled" fields are the isolated error.)

Blast radius — this is not confined to the new tab.

  1. AudioSettings.Default (AudioSettings.cs:72-75) does not override the trailing parameters, so Default.SfxDisabled == Default.AmbientDisabled == true.
  2. SettingsStore.LoadAudio uses d.<field> as the missing-key fallback (SettingsStore.cs:123-124), so every existing settings.json — which has no sfxDisabled key — loads as disabled. Not just fresh profiles.
  3. RuntimeSettingsController.ApplyStartup (:250-263) runs unconditionally in the graphical host (SettingsDevToolsComposition.cs:49) and calls the same static ApplyAudio, so the client comes up with SfxVolume = 0 and AmbientVolume = 0 against engine defaults of 1f / 0.8f (OpenAlAudioEngine.cs:160-161).
  4. The per-frame draft-preview path (WorldRenderFrameBuilder.cs:372) shares the same static helper, so the legacy Settings-panel audio sliders are muted too.

Net effect: silent sound effects and silent ambient audio for all users on the next launch, undoing Campaign A's A2/A5 work.

The defect is enshrined, not merely present. A test asserts it (tests/AcDream.App.Tests/UI/Layout/ConfigOptionsPageControllerTests.cs:41-58, Assert.True(d.SfxDisabled) under a comment calling it byte-verified), the register says so (AP-199: "gating the effective volume to zero when disabled"), and the gate script tells the user this is "retail's own byte-verified 2013 EoR default … not a bug, however counterintuitive" (2026-08-11-campaign-op-test-script.md:567-572).

Fix direction: store retail's own enabled-sense value. Rename to SfxEnabled/AmbientEnabled/InterfaceEnabled (default true), make ApplyAudio enabled ? volume : 0f, and read the DAT caption for ID_Sound_DisableSound at the gate to confirm which way the LED reads — the key name is legacy; the stored semantic is unambiguous. SettingsStore must keep reading the old sfxDisabled key only if it inverts it (or, cleaner, use fresh key names since no shipped profile can contain the one-commit-old key).


M3 — UI_ChatFontFace ships 1 of retail's 5 authored choices, and the row's default index falls outside it

ConfigOptionsPageController.cs:551-554:

private static readonly string[] ChatFontFaceChoices = { "ID_UI_Value_Arial", };

justified at :571-578 with "the face-choice array is authored with only ONE literal decoded so far … the remaining installed system fonts are a per-machine list retail enumerates at runtime (UIPreferences::AttachPreference's own SmartArray grow loop has no fixed upper bound in the decompile)".

That is contradicted by the decompile the slice cites. gmClient::InitUIPreferences builds a fixed, compile-time five-entry array at 0x004038850x004039ED (acclient_2013_pseudo_c.txt:2765-2802): ID_UI_Value_Arial, ID_UI_Value_CourierNew, ID_UI_Value_PalatinoLinotype, ID_UI_Value_Tahoma, ID_UI_Value_TimesNewRoman, then one SetEnumChoices. All five strings are present verbatim in the binary (.rdata string scan). The SmartArray growth is just the container's normal push idiom — the same idiom used for ChatFontSize, which the slice did transcribe as five entries.

Consequence: the menu offers one item, and the row's retail default 2 (:583, matching SetDefaultValue(2) @0x0049E7DE = PalatinoLinotype) indexes past a 1-entry array — a self-inflicted copy of the genuine LandscapeDrawDistance oddity, which makes the real oddity indistinguishable from this bug at the gate.


SHOULD-FIX

S1 — the trailing separator is missing (5 emitted, retail emits 6)

gmConfigUI::InitOptions ends /* tailcall */ return PlayerOptionPage::AddSeperator(this); (0x0049E80D), for six AddSeperator calls: 0x0049E4A9, 0x0049E58A, 0x0049E6A4, 0x0049E750, 0x0049E7C3, 0x0049E80D. Bind (ConfigOptionsPageController.cs:241-251) emits five interior separators and no trailing one, so the list is 38 items where retail builds 39. Lane A called this out explicitly ("5 interior separators + 1 trailing", 2026-08-10-options-panel-structure.md:487). Visible as a missing bottom spacer. No test pins the separator count.

(Aside: lane A's "24 option rows" in that same line is wrong — the decomp has 27 option Add* calls, which is what the slice implements. The slice is right and the doc is stale; worth correcting the doc while fixing the separator.)

S2 — Screen Brightness overloads DisplaySettings.Gamma, which already has a different unit system

ConfigOptionsPageController.cs:410-416 binds Render_ScreenBrightness (retail range [-1, 1], default 0, AttachPreference @0x004043DF + SetPreferenceRange @0x004043F3) onto the pre-existing Gamma field. But Gamma is a multiplier: DisplaySettings.Default.Gamma == 1.0f (DisplaySettings.cs:79, pinned by DisplaySettingsTests.cs:26) and its live control is renderer.SliderFloat("Gamma", ref gamma, 0.5f, 2.0f) (SettingsPanel.cs:220-222, still present until OP9). Two incompatible unit systems on one field:

  • Fresh profile: Gamma = 1.0 normalizes to (1-(-1))/2 = 1.0 → the Config Screen Brightness slider sits pinned at maximum, where retail sits centred.
  • Clicking Defaults writes Gamma = 0 — outside the legacy slider's [0.5, 2.0] range and meaningless as a gamma multiplier.
  • Moving the legacy slider to 2.0 makes the Config row read 1.5 normalized → clamped.

This also contradicts the slice's own stated principle: AP-200 refuses exactly this kind of overload for ChatSettings.FontSize ("Inventing a … mapping without retail evidence would risk silently overwriting … already-live … behaviour"), while AP-198 does it for Gamma. A new ScreenBrightness field (defaulting to retail's 0f) matches every other OP6 row and costs one more SettingsStore line.

S3 — tooltips reach only 12 of 30 rows, though the doc claims the full table was applied uniformly

The class doc (ConfigOptionsPageController.cs:33-36) claims the complete AttachPreference tooltip table (<label>_Help) was transcribed and "applied uniformly". In fact only ApplyLabelAndTooltip (:940-955) sets a tooltip, and it is called only from BuildToggleRow (:653) and BuildTrioRow (:769). SetLabelText (:957-968), used by every slider and menu row, sets no tooltip. So the 9 plain toggles + 3 trio toggles get tooltips and the 10 sliders + 8 menus do not. Retail attaches a _Help string to every preference on this tab — verified at all 27 AttachPreference sites.

Root cause is a capability gap, not just a missed call: TooltipText exists only on UiButton (UiButton.cs:96-100); UiScrollbar/UiMenu/UiText have no tooltip surface. Either add one (GetTooltipText override, the UiElement.cs:480 hook already exists) or correct the doc and file a register row. Do not leave the doc overclaiming.

S4 — Resolution's retail default "800x600" is not a selectable menu item

BuildStringMenuRow populates from DisplaySettings.AvailableResolutions (DisplaySettings.cs:85-93: 1280x720 … 3840x2160) but passes defaultValue: "800x600" (ConfigOptionsPageController.cs:396, correctly decoded from SetDefaultValue(0x3200258) @0x0049E5AC). Clicking Defaults therefore resizes the window to 800×600 (live, via ApplyDisplayWindowState) and leaves the dropdown showing an entry that cannot be re-selected. The comment at :383-390 frames this as faithful, but retail's Display_Resolution menu is populated from enumerated display modes — 800×600 is a real retail mode (gmClient::Init @0x004047AF literally calls Device::ForceDisplayResolution(1, 0x320, 0x258)). The divergence is acdream's hardcoded modern preset list, not retail's default. Cheapest honest fix: add "800x600" to AvailableResolutions.

S5 — the tests pin the safe seams and miss every risky one

Genuinely good: per-record store round-trips (:97-191), the 30-row registration count and full row-type sequence (:270-327), the scoped-scrollbar tests (:481-554), the missing-ListBox and null-resolver degradation paths (:572-595).

Missing exactly where the three MUST-FIXes live:

  • No test on ApplyAudio's effective-volume mapping. The one changed pre-existing test (RuntimeSettingsControllerTests.cs:300-304) only adds "target-audio" to an ordered event list — legitimate and honest, but it asserts that the target was called, never what reached engine.SfxVolume. A three-line test asserting a default-profile startup leaves SFX audible would have caught M2.
  • No label/choice-key conformance table. LabelResolutionFailure… (:588) passes a null resolver, so every string key — including the 1-vs-5 font-face array — is invisible to tests. The Character tab's registry-conformance pattern (plan §4 OP4: "an invented row or a dropped row fails the build") is the precedent to copy here.
  • No per-row default-value conformance. Defaults_Restores… (:442-469) only asserts self-consistency (Current == DefaultValue), never that DefaultValue equals retail's literal. Row defaults are inline literals with no pin.
  • No separator/list-item-count pin (S1 would have failed one).
  • AudioSettings_Default_… (:41-58) actively pins M2's inverted semantics.

NOTE

  • N1 — AP-198 row count is internally inconsistent. The register row enumerates ten rows ("plus Screen Brightness"); the commit message says "the nine Graphics/Rendering-Quality rows". Reconcile. (The 137 → 140 active-row header count itself is correct: AP-198/199/200 are three new rows, and the TS-74 extension is an edit, not a new row.)
  • N2 — the scrollbar id is hardcoded where the DAT supplies it. The Config ListBox node carries the authored P0x72 value (ScrollbarElementId: 268435969 = 0x10000201 in Fixtures/options_2100002B.json), but the controller hardcodes the constant (:140). It matches today; reading the authored field is the standing project preference (feedback_prefer_dat_field_over_geometry).
  • N3 — Defaults() performs one full settings.json write per row. Each row's apply is Save<Section>(Load<Section>() with { … }), so a Defaults click issues up to 30 sequential load-modify-save round-trips (plus 30 ApplyAudio/ApplyDisplayWindowState pushes, including repeated window resizes). Correctness is fine; it is worth a batched-write follow-up before OP9. Retail's preference system writes the INI on its own cadence, not per row.
  • N4 — AfterApply fires on Config-tab entry. OptionsPanelController.cs:150-154 gives the Config page the same SaveCharacterOptionsRuntimeCmd publisher as Character/Chat. This is retail-shaped — gmConfigUI : PlayerOptionPage, and PlayerOptionPage::SaveCurrentValues @0x004F2710 does call CPlayerModule::SaveToServer(module, 0) — and acdream's flush is dirty-gated ("a clean module sends nothing", RuntimeCharacterState.cs:930), so this is correct, not a finding. Recorded so a later reviewer does not re-open it.

Verified correct (spot-checked against primary sources, no action needed)

These were checked end-to-end and are right; they are the reason this REJECT is narrow rather than a rework.

Template mechanism — all eight indices verified against both the decomp and the committed fixture (Fixtures/options_2100002B.json, ListBox 0x10000200 TemplateList):

idx Fixture element Retail call Slice constant
0 0x10000216 (Type 12) AddHeaderAddItemFromTemplateList(…, 0) @0x004F28C5 HeaderTemplateIndex
1 0x10000217 AddSeperator(…, 1) @0x004F273E SeparatorTemplateIndex
2 0x10000218 + 0x10000219 AddToggleOption(…, 2), child 0x10000219, cast 0x10000035 @0x004F29A3 ToggleTemplateIndex / ToggleCheckboxElementId
3 0x1000021A + 0x1000021B/0x1000021C AddSliderOption(arg3=0)(…, 3) SimpleSliderTemplateIndex
4 0x10000222 + 0x10000223/0x10000224 AddMenuOption(…, 4), child 0x10000224, cast 0x10000038 @0x004F2B03 MenuTemplateIndex
5 0x10000220 (Type 0x10000036) AddToggleWithSliderOption(…, 5) @0x004F2943 TrioTemplateIndex
6 0x1000021D (+ caption children) AddSliderOption(arg3=1)(…, 6) RangedSliderTemplateIndex
7 0x10000221 never invoked by InitOptions correctly unused ✓
  • The AddSliderOption arg3 → template decode is a correct BN-artifact read. BN renders AddItemFromTemplateList(m_pOptionBox, (((eax - eax) & 3) + 3)) (@0x004F2A90) — a mangled neg/sbb/and 3/add 3, i.e. arg3 != 0 → 6, arg3 == 0 → 3. The slice's 3-vs-6 split, and its "idx3 is used exactly once" structural cross-check, are both right.
  • AddMenuOption's arg3 does NOT change the template (always 4); it selects SetUIPreference vs SetUserPreference (@0x004F2B3C/@0x004F2B67). The slice's decision to use one template with two payload types (int / string for Resolution) matches.
  • Every slider range matches UIPreferences::SetPreferenceRange exactly: volumes [0,1]; Camera_Stiffness [0.285714298, 1] @0x0040465B; Camera_AdjustmentSpeed [5, 80] @0x0040469C; Render_FieldOfView [10, 160] @0x004043B2; Render_ScreenBrightness [-1, 1] @0x004043F3; Render_GraphicsPerformance [-1, 1] @0x00404461; Render_DegradeDistance [0, 100] @0x0040449E; Input_MouseLookSensitivity [0.00999999978, 1] @0x00404593.
  • Every label key is right, including all six the pseudo-C truncates. Verified by scanning the binary's ID_* strings: …TextureFiltering_Sharp, …BuildingDetailTextures, …LandscapeTextureDetail, …EnvironmentTextureDetail, …LandscapeDrawDistance, ID_Rendering_SyncToDisplayRefresh. The _Help tooltip-suffix convention is correct at all 27 AttachPreference sites (cf. Chat's _Desc).
  • Every numeric default matches, incl. 0x3EE66666 = 0.45, 0x42200000 = 40, 0x42B40000 = 90, 0x42480000 = 50, 0x3F0CCCCD = 0.55, 0x03200258 = 800x600, and the four menu immediates (6a 02 / 6a 01 / 6a 01 / 6a 08 at 0x0049E6C60x0049E714).
  • The LandscapeDrawDistance oddity is real and reproduced faithfully. SetDefaultValue(8) (6a 08 @0x0049E714) against a 6-entry SetEnumChoices (VeryLow…Extreme, @0x00404371). Stored as an opaque int; UiMenu handles the out-of-range value safely (Equals(Payload, 8) matches nothing → no highlight, and ButtonLabelProvider bounds-checks at :853-857). AP-198's sub-note is accurate. This is the one place where the "reproduce, don't fix" instinct was applied correctly — M1/M3 are the same instinct applied to non-facts.
  • Row order matches InitOptions statement-for-statement across all six sections (only the trailing separator, S1, differs).
  • The scoped-lookup fix is correct and its test genuinely discriminates. Scoping from PageSlotElementId = 0x10000213 (:224-227) is exactly what OP5's review N11 demanded, and SharedScrollbarId_ChatAndConfigBoundTogether_… (:508-554) asserts through scoped lookups on both pages plus NotSame(chatListBox.Scroll, configListBox.Scroll) — under a flat lookup Config's Bind would overwrite the single shared instance's Model and the Chat assertion fails. This is a strictly better test than the OP5 one N11 criticised.
  • IntOptionRow/StringOptionRow match the Bool/Float contract exactly (OptionPageModel.cs:282-434): same (current, saved, default) triple, same SetCurrentValue → apply + notify / SaveCurrentValue → read + refresh / RestoreSaved / RestoreDefault split, and AttachPageNotify so both participate in Apply/Reset ghosting (OptionsPanelController.cs:229-243). Every Config row supplies read/refresh, so the OP4 re-seed (OnShown → Apply) and the R2 ReloadFromLive path both work. StringOptionRow.Changed uses C# string value equality — correct.
  • The ApplyAudio seam reaches the real engine and does not double-apply at startup. RuntimeSettingsTargets._audiocontent.Audio?.Engine (SessionPlayerComposition.cs:347) is the same OpenAlAudioEngine instance as the startup targets (:364) and the frame preview (FrameRootComposition.cs:355). ApplyStartup runs once via the startup target; SaveAudio only fires on an actual save; Bind only reads. The optional-trailing ctor parameter keeps every pre-existing construction site compiling. (The mechanism is right — only the value it pushes is wrong, per M2.)
  • Interface addition blast is contained. IRuntimeSettingsTargets has exactly one production implementor (RuntimeSettingsTargets, internal sealed) and one test fake, both updated; the startup interface's separate fake is untouched.
  • Store schema is additive-only with correct fallbacks. All new keys go through ReadBool/ReadFloat/the new ReadInt with the record default as fallback (SettingsStore.cs:73-81, 121-126, 217-218, 255), writers extend the existing sorted dictionaries, no version field, no removals — an existing settings.json parses unchanged. (The only behavioural break is M2's default, not the schema.)
  • No invented user-visible text. Every caption resolves from string table 0x23000003 by compute_str_hash name; failures log and leave the widget blank (:607-615, :840-848, :943-955, :957-968), matching the campaign's §6 rule and the Character/Chat controllers' convention.
  • TS-74's extension is accurate and correctly scoped (the sixth, previously-unmodeled Input_UseMouseTurning client-local preference, correctly distinguished from the server-synced PlayerOption.UseMouseTurning bit), and the AP-174 citation on the Interface trio is correct — retail registers Sound_InterfaceSoundVolume/…Disabled (@0x004036E0/@0x004036B3) and never reads them; AP-174 already documents "interface sounds are scaled by the EFFECT knob". Matching retail's dead knob rather than building a working one is the right call.

Fix checklist

# Severity Item
M1 MUST Wire the six range-caption pairs (Soft/Hard, Slow/Fast, Narrow/Wide, Dark/Bright, Speed/Detail, Close/Far) onto idx6's 0x1000021E/0x1000021F; correct the class doc, ledger/commit claim, and gate-script lines 535 + 653
M2 MUST Invert the three Sound toggles to retail's enabled-sense (effect_sounds_enabled); fix AudioSettings defaults, ApplyAudio, the store keys, the pinning test, AP-199's wording, and gate-script step 6
M3 MUST Ship all five UI_ChatFontFace choices; delete the "runtime-enumerated" justification
S1 SHOULD Emit the trailing separator (6 total); pin the count; correct lane A's "24 option rows"
S2 SHOULD Give Screen Brightness its own field instead of overloading Gamma
S3 SHOULD Tooltips on slider/menu rows, or correct the doc + file a register row
S4 SHOULD Add "800x600" to AvailableResolutions so Defaults lands on a selectable item
S5 SHOULD Add: effective-engine-volume test, label/choice-key conformance table, per-row retail-default pin, separator-count pin
N1N4 NOTE AP-198 nine-vs-ten; read the authored scrollbar id; batch the Defaults writes; (N4 needs no action)