# 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).