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