acdream/docs/research/2026-08-11-op9-review.md

# Campaign OP slice OP9 — combined-lens review (mechanism × blast radius)

**Target:** `371197a3` — *refactor(settings): OP9 — retire the dead F11
settings surface + fully-superseded GameplaySettings*
**Reviewer:** combined mechanism-faithfulness × regression/blast-radius pass
(one reviewer, OP5/OP7 precedent for closeout-shaped slices).
**Base:** `1a57f96e` (campaign tip after the OP8 merge). Worktree clean at
review time; HEAD == the target commit.

## VERDICT: APPROVE-WITH-FIXES

The retirement itself is correct. Every claim in the commit message's
per-file disposition list that bears on **live behavior** verifies against
the post-commit source: no production code path read `GameplaySettings`, no
production code path constructed `SettingsVM`, `SetUiLocked`'s observable
contract is unchanged, the wire behavior of `SetAcceptLootPermits` /
`ToggleUiLock` / the PlayerDescription convergence is untouched, and the
settings.json unknown-key carry-forward mechanism is real. The register
edit is arithmetically and semantically correct.

Five findings, none of them a behavior regression: one lost regression
guard on a live mechanism (MUST-FIX 1), one stale **architecture** doc
seam (SF-2), three dead residues the retirement created and left (SF-3),
a test-delta claim that does not reconcile (SF-4), and a dangling
comment (SF-5). Plus two nits.

---

## MUST-FIX 1 — `SaveAudio` → `ApplyAudio` (OP6's live-apply) lost its only test

`RuntimeSettingsController.SaveAudio` pushes the saved snapshot into the
live engine — `src/AcDream.App/Settings/RuntimeSettingsController.cs:420`
(`_runtimeTargets?.ApplyAudio(audio);`), documented at `:407-413` as the
Campaign OP slice OP6 change that made Audio-tab edits take effect
without a relaunch. Its live production consumer is
`ConfigOptionsPageController`'s Sound rows (`ConfigOptionsPageController.cs:103`).

Pre-commit, exactly one test asserted that push: the now-deleted
`SettingsViewModelSavePreservesSectionAndTargetOrder`, whose ordered event
list contained `"target-audio"` with an explicit OP6 comment
(`371197a3~1:tests/AcDream.App.Tests/Settings/RuntimeSettingsControllerTests.cs:296-312`).

Post-commit, `grep -rn ApplyAudio src tests` shows **no assertion anywhere**
that `SaveAudio` invokes `IRuntimeSettingsTargets.ApplyAudio`. The fake
still records the event —
`tests/AcDream.App.Tests/Settings/RuntimeSettingsControllerTests.cs:817-820`
(`events.Add("target-audio");`) — and nothing ever reads it. The two
surviving audio tests (`ComputeEffectiveCategoryVolumes_*`, `:241-278`)
test the static volume helper, not the controller push. `ApplyStartup`'s
own audio call (`RuntimeSettingsController.cs:226`) is still covered by
`StartupApplyIsOrderedExactlyOnceAndRuntimeBindingDoesNotReplay` (`:54`),
so this gap is specific to the **save** path.

This is the class the slice contract cannot absorb: the commit asserts
"no behavior change to anything live, verified", and one live mechanism is
now unverified. Note the contrast — the *other* three target pushes were
kept covered, deliberately or not:

| Push | Post-commit coverage |
|---|---|
| `SaveDisplay` → `ApplyDisplayWindowState` | implicit but real — `DisplayTargetFailurePreservesEstablishedStoreThenPublishBoundary` (`:566`) sets `ThrowOnDisplay` and asserts the resulting state boundary, which only happens if the target was called |
| `ReapplyQualityPreset` → `ApplyQuality` | positive (`:633`, `:653`) |
| `SetUiLocked` → `ApplyUiLock` | positive (`:672-679`, `:694`, `:702`) |
| `SaveChat` → `SetChatOpacity` | positive — `SaveChat_PushesOpacityToRuntimeTargets_LiveApply_NoRestart` (`:338`) |
| **`SaveAudio` → `ApplyAudio`** | **none** |

**Fix (minimal):** in `RuntimeSettingsControllerTests`, add the missing
positive assertion in the same shape as the chat-opacity test — bind a
`FakeRuntimeTargets`, call `controller.SaveAudio(controller.Audio with
{ Master = 0.25f })`, assert the recorded events contain `"target-audio"`
(ideally assert the *value* forwarded, per the `:236-240` comment's own
lesson about M2's inversion).

---

## SHOULD-FIX 2 — the architecture doc still names `SettingsVM` as a live seam

`docs/architecture/code-structure.md:209-212`:

> `WorldRenderFrameBuilder -> RuntimeWorldFrameSettingsPreview ->
> IRuntimeSettingsPreviewSource -> RuntimeSettingsController -> optional
> SettingsVM` for the live settings draft preview applied before world drawing.

This sits inside a present-tense list of current presentation seams
("These are presentation state/command seams…", `:217`), not a historical
narrative. It is the one surviving reference that would send a future
reader looking for a type that no longer exists, and CLAUDE.md's
architecture rule ("when the architecture doc and reality diverge, update
one or the other") makes it in-scope for the same commit.

Secondary, lower severity, same file: `:738-739` — "The controller also
preserves unrelated unsaved Gameplay draft fields when combat preferences
change" is now false. It is inside the Slice-8 checkpoint-G/H ledger
narrative, so it reads as dated history; a one-clause "(retired at
Campaign OP slice OP9)" is enough.

Everything else the repo-wide sweep turned up is genuinely historical and
correctly left alone: `docs/plans/2026-07-22-gamewindow-slice-8-*.md`,
`memory/project_gamewindow_decomposition.md:81/85/181`,
`docs/research/2026-08-1*-op{3,4,6,8}-*.md`,
`docs/research/2026-08-10-keyboard-config-and-gameplay-tab.md`,
`docs/research/2026-08-10-character-options-map.md`,
`docs/ISSUES.md:15859`, and the `docs/research/2026-05-21-a6-captures/*.log`
run logs.

---

## SHOULD-FIX 3 — three dead residues left behind by the retirement

The slice deleted the *callers* without sweeping what became unreachable:

1. **`RuntimeSettingsController.SaveCharacter(CharacterSettings)`** —
   `src/AcDream.App/Settings/RuntimeSettingsController.cs:547-566`. Private,
   **zero callers** post-commit (its only caller was the `saveCharacter`
   callback handed to the deleted `CreateViewModel`). C# emits no warning
   for an unused private method, so the build stays green while the method,
   its `_defaultCharacter` promotion branch, and the whole write half of
   the `CharacterSettings` store are unreachable:
   `IRuntimeSettingsStorage.SaveCharacter` (`:31`),
   `JsonRuntimeSettingsStorage.SaveCharacter` (`:68`), and
   `SettingsStore.SaveCharacter` now have no production writer.
   (`ActiveToonKey` is genuinely live — `InteractionRetainedUiComposition.cs:507`
   and `:612` key per-character window layouts off it — so only the
   `Character`/`SaveCharacter` half is dead. `RuntimeSettingsController.Character`
   likewise has no production reader.)
   The commit also deleted that method's only test
   (`CharacterSaveUsesActiveToonAndDefaultSaveBecomesResetContext`) without
   re-targeting it — consistent with the code being dead, but it means the
   code is now both uncallable and untested rather than removed.
2. **`IngressShutdownRoots.Settings`** —
   `src/AcDream.App/Rendering/GameWindowLifetime.cs:75`, captured at
   `src/AcDream.App/Rendering/GameWindow.cs:1670`. Its **only** reader was
   the `Soft("settings view model", …UnbindViewModel())` stage this commit
   deleted; `grep` finds no other `ingress.Settings` use. Harmless (a
   reference held until shutdown) but it is now a shutdown-roots member
   that roots nothing.

Either delete these in the OP9 follow-up, or — if `CharacterSettings`
persistence is meant to come back on a later Options-panel tab — say so in
a one-line comment so the next reader does not re-derive that it is dead.
The commit message's "CharacterSettings … UNTOUCHED per the campaign
contract" covers keeping the *record and store*; it does not explain the
now-uncallable controller method.

---

## SHOULD-FIX 4 — the −80 test delta does not reconcile; static enumeration gives −84

The commit states the delta "is exactly the deleted SettingsVM/
SettingsPanel/GameplaySettings test surface". Enumerating the touched
files at both commits:

| File | Removed | Added |
|---|---:|---:|
| `tests/.../Panels/Settings/GameplaySettingsTests.cs` (deleted) | 3 | — |
| `tests/.../Panels/Settings/SettingsVMTests.cs` (deleted) | 43 | — |
| `tests/.../Panels/Settings/SettingsPanelTests.cs` (deleted) | 29 | — |
| `tests/AcDream.App.Tests/Settings/RuntimeSettingsControllerTests.cs` | 13 | 6 |
| `tests/.../Panels/Settings/SettingsStoreTests.cs` | 4 | 2 |
| `tests/AcDream.App.Tests/Input/GameplayInputCommandControllerTests.cs` | 2 (InlineData) | 2 (InlineData) |
| `tests/.../Rendering/GameWindowSlice8BoundaryTests.cs`, `tests/.../Input/InputDispatcherCaptureTests.cs` | 0 | 0 |
| **Net** | **94** | **10** |

**−84**, not −80. All three deleted files declare `public sealed class`
(discoverable), none carries `[Theory]`/`[MemberData]`/`Skip`, and the
skip count is 4 on both sides — so no discovery artifact explains the gap.
The baseline is quoted twice in the immediately preceding commits
(`f1d50207`, `1a57f96e`: 13,155 passed / 4 skipped), so either that
baseline is off by 4 or the post number is. Nothing in the diff adds four
tests elsewhere.

This is an evidence-accounting defect, not a behavior defect — **no live
behavior test is unaccounted for by it** (the one real coverage loss is
MUST-FIX 1, which is inside the 13-removed RuntimeSettingsControllerTests
bucket). Re-measure and restate, or drop the word "exactly".

For the record, the 13 removed `RuntimeSettingsControllerTests` cases
disposition cleanly: 6 were re-targeted onto the surviving API
(`CharacterContext*`, `DraftPreview*`, `RuntimeTargetLoans*`,
`SetUiLocked_*`, `SyncChatFromServerOptions*`, `UnboundRuntimeTargets*`),
5 were SettingsVM-draft-only, 1 (`UiLockPersistenceFailureCanRetry…`)
pinned a retry against a save that no longer exists — and its
target-failure sibling `UiLockTargetFailureCanRetryTheSameRequestedValue`
(`:683`) survives and still pins the real invariant — 1 covered the now-dead
`SaveCharacter` (SF-3), and 1 (`SettingsViewModelSavePreservesSectionAndTargetOrder`)
carried the `"target-audio"` assertion (MUST-FIX 1).

---

## SHOULD-FIX 5 — dangling comment referencing a deleted assertion

`tests/AcDream.App.Tests/Settings/RuntimeSettingsControllerTests.cs:236-240`:

> "The FakeRuntimeTargets-based `"target-audio"` assertion **above** (and its
> predecessor before this rework) only ever checked that ApplyAudio fired…"

There is no longer any `"target-audio"` assertion above (or anywhere); what
is now immediately above is
`ConcreteRuntimeTargetPublishesSetSingleCharacterOptionOntoTheBus`. Reword
when fixing MUST-FIX 1 — the two are the same edit.

---

## NIT 6 — "bound through … `CharacterOptionsPageController`" is imprecise for 2 of the 13

Commit §4 and the AP-196 retirement note both say all 13 members "were
ALREADY bound through `CharacterOptionTable`/`CharacterOptionsPageController`'s
server-bit seam (the OP4 Character tab)". All 13 are in the table —
verified one by one at `src/AcDream.Runtime/Gameplay/CharacterOptionTable.cs`
`:117,119,121,123,125,127,130,131,132,158,160,161,162` — but **two are
deliberately not Character-tab rows** (`CharacterOptionsPageControllerTests.cs:26`
names the exclusion set: `AppearOffline`, `UseMouseTurning`, `LockUI`, per
research §2.7):

- `LockUI` — bound through `/lockui` + the radar control:
  `LiveSessionRuntimeFactory.cs:419-421` (server-bit read, `0x0005` send,
  `SetUiLocked` visual push), `InteractionRetainedUiComposition.cs:492`
  (mount-time seed), `:647` (radar binding).
- `UseMouseTurning` — bound through the Gameplay-tab mouse macro:
  `InteractionRetainedUiComposition.cs:843-845` (`IsUseMouseTurningEnabled`
  live server-bit read) and `RetailUiRuntime.cs:2009-2019` (wire send).
  Note this is the *server* bit, correctly distinct from the Config tab's
  own client-local `CameraTurningSettings.UseMouseTurning` (TS-74,
  `ConfigOptionsPageController.cs:605-612`).

The **substance holds** — all 13 have a live server-bit home and none was
orphaned — only the sentence's channel attribution is loose. Worth
correcting in the register row, which is the durable artifact.

## NIT 7 — OP9's own bookkeeping is still outstanding

`docs/plans/2026-08-10-options-panel-campaign.md:428` still reads
`| OP9 | — | | | |`, and `:9` still says "ACTIVE — planned 2026-08-10. No
slices landed yet" (stale for the whole campaign, pre-existing). §OP9's
bullet list (`:352-370`) also owes the ISSUES sweep (#358 retest), the
CLAUDE.md Current-state paragraph, and the memory digest update. None of
that is a defect in *this* commit — it is the closeout half of the slice —
but it should not be forgotten when OP9 is declared complete.

---

## Verified clean (mechanism lens)

1. **`RuntimeSettingsController` live core** — all present and unaltered:
   storage load/save for Display/Audio/Chat/Character (`:170-181`),
   `ResolvedQuality`/`ReapplyQualityPreset` (`:369-383`), `ApplyStartup`
   (`:213-235`), `SaveAudio`'s OP6 live-apply (`:414-427`), public
   `SaveDisplay` (`:391-405`, still the `ConfigOptionsPageController` seam),
   `IRuntimeSettingsTargets.SetSingleCharacterOption` (`:115`) and
   `SetChatOpacity` (`:124`), `ServerOptionsSeeded` /
   `NotifyServerOptionsSeeded` (`:542-545`), `SetUiLocked` (`:292-299`).
   The only removals are `Gameplay`, `SetAcceptLootPermits`, the private
   `SaveGameplay`, and the four `_viewModel?` notification calls.

   **`SetUiLocked` first-call / repeat-call equivalence — verified.**
   The old guard was `if (_lastAppliedUiLocked == locked && _uiLockConverged)
   return;` with `_uiLockConverged` initialised **true** and
   `_lastAppliedUiLocked` initialised **null**. On the first call
   `(bool?)null == locked` is `false`, so the conjunct never mattered: the
   first `ApplyUiLock` push happened then and happens now. Deleting
   `_uiLockConverged` therefore cannot have changed whether the first push
   fires. Repeat same-value calls returned early then (after a successful
   `SaveGameplay` set `_uiLockConverged = true`) and return early now.
   Failure ordering is also preserved: `_lastAppliedUiLocked` is still
   assigned *after* `ApplyUiLock`, so a throwing target leaves the guard
   un-advanced and the same value can be retried — pinned by
   `UiLockTargetFailureCanRetryTheSameRequestedValue` (`:683-704`). The one
   behavioral difference is that a *storage* failure no longer forces a
   redundant re-push on the next same-value call; there is no storage call
   left to fail, and `ApplyUiLock` is idempotent. All three call sites
   traced and unchanged: PlayerDescription convergence
   (`LiveSessionRuntimeFactory.cs:339-340`), `ToggleUiLock` (`:419-421`),
   radar/Character-tab path (`InteractionRetainedUiComposition.cs:647`).

2. **`IRuntimeSettingsPreviewSource`** — the "already always false in
   production" claim is **true**. Searching the *pre*-commit tree
   (`git grep … 371197a3~1`) for `CreateViewModel` / `CreateViewModelBinding`
   / `new SettingsVM` / `new SettingsPanel` returns **only** test files plus
   the controller's own definitions. `_viewModel` was therefore always
   `null` in production, so `HasDraftPreview` was always `false` and
   `DisplayPreview`/`AudioPreview` always fell through to
   `Display`/`Audio` — exactly what they now return unconditionally. Both
   consumers see identical values: `WorldRenderFrameBuilder.cs:370-380`
   (the whole draft block was already dead in production) and
   `LiveObjectFrameController.cs:59-62` (`SettingsParticleRangeSource`,
   which reads `DisplayPreview` unconditionally — unchanged). Third
   consumer `InteractionRetainedUiComposition.cs:685`
   (`DisplayPreview.ShowFps`) likewise unchanged.

3. **`GameplayInputCommandController`** — diff confirms exactly three cases
   touched. `AcdreamToggleDebugPanel` (`:190-197`) and `ToggleChatEntry`
   (`:208-213`) keep `return true`, matching the prior no-op's handled
   contract; `ToggleOptionsPanel` (`:214-224`) still calls
   `_retained.ToggleOptionsPanel()` with only the comment reworded. No
   other `InputAction` case changed, and the removed constructor parameter
   is the only signature change (`SessionPlayerComposition.cs:1194`
   updated). Worth recording: the return value is inert in production —
   `GameplayInputActionRouter.HandleCommand` (`:170-171`) discards it and
   the command controller is the **last** stage of the priority graph — so
   the `true`-vs-`false` question could not have been observable either
   way. The new `RetiredDevToolsCommand_IsConsumedWithoutClaimingATypedOwner`
   theory (`GameplayInputCommandControllerTests.cs:43-56`) re-targets the two
   deleted mapping rows rather than dropping them. No `ToggleSettingsPanel`
   `InputAction` exists (`grep` over `AcDream.UI.Abstractions` is empty), so
   `KeyBindings.RetailDefaults()` and the keymap fixtures needed no edit —
   claim confirmed.

4. **`LiveSessionRuntimeFactory`** — `SetAcceptLootPermits` still sends
   `0x0005` via `SendSingleCharacterOption` (`:494-496`); only the
   `_interaction.Settings.SetAcceptLootPermits(value)` write-behind line was
   removed, and it had no reader. `AcceptLootPermits` still reads the server
   bit (`:491-493`). `ToggleUiLock` (`:415-422`) and the PlayerDescription
   `OnCharacterOptionsChanged` convergence (`:337-345`) are byte-identical
   apart from comments.

5. **settings.json migration** — mechanism verified at
   `SettingsStore.SaveSection` (`src/AcDream.UI.Abstractions/Panels/Settings/SettingsStore.cs:643-687`):
   every top-level key other than the target section and `"version"` is
   carried forward by `prop.Value.GetRawText()`, so an orphaned
   `"gameplay"` block survives *every* subsequent save, and no `Load*`
   method reads it. The two new tests
   (`SettingsStoreTests.cs:219-260`) pin exactly that, not something weaker:
   the first proves a leftover block does not disturb Display/Audio/Chat
   loads; the second writes an unrelated section and asserts the raw
   `"gameplay"` / `"lockUI": true` / `"showHelm": false` text is still in the
   file afterwards. The re-targeted coexistence and window-position/layout
   tests correctly substitute a Display assertion for the deleted Gameplay
   one, preserving each test's actual point.

## Verified clean (blast lens)

- **Forward references.** Repo-wide sweep (`src`, `tests`, `tools`,
  `scripts`, `docs`, `.claude`) for `SettingsVM`, `SettingsPanel`,
  `RuntimeSettingsViewModelBinding`, `CreateViewModel`,
  `IDevToolsGameplayCommands`, `DevToolsGameplayCommands`,
  `GameplaySettings`, `LoadGameplay`, `SaveGameplay`,
  `SetAcceptLootPermits` (the controller method), `ToggleSettingsPanel`,
  `UnbindViewModel`: the only *code* hits are OP9's own explanatory
  comments, plus `ICombatGameplaySettingsSource` substring matches
  (`RuntimeCharacterState.cs:850`) and an unrelated
  `DebugVM CreateViewModel()` local helper in
  `tests/AcDream.App.Tests/Combat/CombatFeedbackSlotTests.cs:29`. One
  vacuous-but-harmless residue: `GameWindowSlice8BoundaryTests.cs:319`
  still asserts `DoesNotContain(".LoadGameplay()")` (and `:328/:330`
  `_persistedGameplay`/`_settingsVm`) — negative source-text guards that
  can no longer fail wrongly; leaving them is fine.
- **No string/reflection lookup** of the deleted panel: `acdream.settings`
  (the old `SettingsPanel.Id`) appears nowhere in the repo.
- **`IPanel`/`IPanelRenderer` survive with real implementers** —
  `ChatPanel.cs:34`, `DebugPanel.cs:22`, `VitalsPanel.cs:16`. Claim
  confirmed.
- **Headless / UI Studio / plugin surfaces** (the three the implementer's
  15-file list did not cover): `src/AcDream.Headless`,
  `src/AcDream.Plugin.Abstractions`, `src/AcDream.Runtime`,
  `src/AcDream.Core*`, `tools/`, `scripts/`, `.claude/` — **zero** hits for
  any deleted type. `src/AcDream.UI.ImGui/` is an empty leftover directory,
  not in `AcDream.slnx`.
- **`SettingsDevToolsComposition` survives correctly.** Despite its name it
  is a live composition phase (`ApplyStartup`,
  `SettingsDevToolsComposition.cs:53`); only its doc comment needed the
  update it got. The plan's "retire … `SettingsDevToolsComposition` wiring"
  bullet referred to the ImGui wiring already removed at V11.
- **Register (§7 of the brief).** `AP-196` is struck through with a
  substantive retirement note (`retail-divergence-register.md:192`), the
  section-3 header moves 143 → 142 (`:178`), and the counts verify by
  enumeration: active `| AP-` rows 143 → 142, retired `| ~~AP-` rows
  29 → 30, total row count unchanged. No other row cites a deleted type —
  the only other `GameplaySettings` hit is `TS-73` (`:367`), a substring
  match inside `ICombatGameplaySettingsSource`.
- **Gate script (§9 of the brief).**
  `docs/research/2026-08-11-campaign-op-test-script.md` is clean: every
  "F11" and "Gameplay tab" reference is to the retail Options panel
  (post-OP3), not to the deleted ImGui surface. No step exercises anything
  this commit removed.

---

## Fix list (ordered, for the fixer)

1. Add the `SaveAudio` → `ApplyAudio` assertion to
   `RuntimeSettingsControllerTests`; reword the `:236-240` comment in the
   same edit (MUST-FIX 1 + SF-5).
2. Update `docs/architecture/code-structure.md:209-212` (drop the
   `-> optional SettingsVM` link) and tag `:738-739` as OP9-retired (SF-2).
3. Delete `RuntimeSettingsController.SaveCharacter` (`:547-566`) and
   `IngressShutdownRoots.Settings` (`GameWindowLifetime.cs:75`,
   `GameWindow.cs:1670`), or document why they stay (SF-3).
4. Re-measure and restate the test delta, or drop "exactly" (SF-4).
5. Correct the LockUI/UseMouseTurning channel wording in the AP-196
   retirement note (NIT 6); fill the OP9 plan-ledger row at closeout
   (NIT 7).