# Campaign OP slice OP8 (Configure Keyboard) — combined dual-lens review **Reviewed:** worktree `C:\Users\erikn\source\repos\acdream\.claude\worktrees\op8-keyboard`, branch `op8-keyboard`, commit `b4edee97`, based on campaign tip `ff577641`. **Contract:** `docs/plans/2026-08-10-options-panel-campaign.md` §4 OP8 + §2 D4. **Research:** `docs/research/2026-08-10-keyboard-config-and-gameplay-tab.md` §5–§7. **Mode:** read-only. No build, no test, no launch. Findings are source- and DAT-fixture-derived, plus independent decomp corroboration. ## Verdict: **REJECT** Three MUST-FIX findings. Two are silent data-destruction bugs in the live `KeyBindings` write path that the slice's own test seam cannot see by construction (M1, M2); one is a contract deviation where the shipped behaviour is strictly worse than the register row describing it, and the connected-gate script was rewritten to accept it (M3). The DAT-reading half of this slice is genuinely good work — the `0x26000000` resolution is correct and better-anchored than its own citation claims, the "no Clear button" finding is verified against the committed fixture, the six-page scoping avoids the reused-id trap correctly, and the identity table's DAT-vs-`RetailDefaults()` agreement is pinned by a permanent test. The defects are all downstream, in the seam between a resolved row and the live dispatcher. --- ## MUST-FIX ### M1 — every write through `SetForAction` destroys `ActivationType` and `InputScope` `src/AcDream.App/UI/RetailUiRuntime.cs:2337-2344`: ```csharp SetForAction: (action, chords) => { KeyBindings updated = CloneWithout(dispatcher.Bindings, action); foreach (KeyChord chord in chords) updated.Add(new Binding(chord, action)); dispatcher.SetBindings(updated); }, ``` `Binding` (`src/AcDream.UI.Abstractions/Input/Binding.cs:14-17`) defaults `Activation = ActivationType.Press` and `Scope = InputScope.Game`. The seam type (`Action>`, `KeyboardConfigController.cs:158`) carries chords only, and the read side (`RetailUiRuntime.cs:2335-2336`) does `.Select(b => b.Chord)` — so activation and scope are dropped on read and re-invented as `Press`/`Game` on write. That is not hypothetical. `KeyBindings.RetailDefaults()` binds, and `RetailActionIdentityTable` maps, all of the following: | Action | RetailDefaults | Identity-table row | |---|---|---| | `MovementWalkMode` | `ActivationType.Hold` (`KeyBindings.cs:173-174`) | `(0x4, 0x32)` | | `CombatLowAttack` / `MediumAttack` / `HighAttack` | `Hold` + `InputScope.MeleeCombat` (`KeyBindings.cs:262-264`) | `(0x10000003, 0x1000005D-5F)` | | `CombatDecrease/IncreaseAttackPower` | `InputScope.MeleeCombat` (`:257-258`) | `(0x10000003, 0x1000005B-5C)` | | every Missile row | `InputScope.MissileCombat` (`:269-273`) | `(0x10000004, 0x100000EF-F3)` | | every Magic row | `InputScope.MagicCombat` (`:274-287`) | `(0x10000005, …)` | | `CameraInstantMouseLook` | `Hold`, mouse `Device: 1` (`:303-306`) | `(0x5, 0x3D)` | Blast radius is much wider than "a user rebinds one key", because `ActionKeyMapOptionRow` invokes `_apply` from `SetCurrentValue`, `RestoreSavedValue` **and** `RestoreDefaultValue` (`OptionPageModel.cs:566-587`): - **Cancel / Revert** run `Page.Reset()` → `RestoreSavedValue()` → `_apply`, so cancelling a rebind does *not* restore the original `Hold`/scope — it writes the saved chords back as `Press`/`Game`. - **Defaults** runs `Page.Defaults()` → `RestoreDefaultValue()` on **every** row unconditionally (`OptionPageModel.cs:712-717`), so a single click of Defaults collapses the activation and scope of the whole mapped table — ~140 actions — in one shot. `KeyboardConfigController.WireScreenButtons` (`:476-484`) wires exactly that. - **OK** then persists the collapsed table: `dispatcher.Bindings.SaveToFile(...)` (`RetailUiRuntime.cs:2349`), and `KeyBindings.SaveToFile` only emits activation/scope when non-default (`KeyBindings.cs:514-516`), so the loss is permanent across relaunch. Observable effects: walk-mode never unlatches (its `Hold` becomes `Press`); the three melee attack-height holds stop repeating; every combat-scoped binding moves into `InputScope.Game`, changing scope precedence for every other binding sharing those chords (Insert/Delete/End/PageDown/PageUp are shared across Melee/Missile/Magic by design); and `CameraInstantMouseLook`'s `Device: 1` MMB-hold is replaced by a keyboard `Press` chord. Note the connected-gate script step 12 (`docs/research/2026-08-11-campaign-op-test-script.md`, §OP8 "Erase, then Reset to Defaults") instructs the user to click Defaults — i.e. the gate as written triggers this bug. **Fix shape:** widen the seam to carry `Binding`, not `KeyChord` — read `dispatcher.Bindings.ForAction(action)` whole, and on write preserve each surviving binding's `Activation`/`Scope`, defaulting new chords from the action's existing bindings (or from the DAT row's `RetailKeyChord.Activation`, which `RetailActionMapReader` already reads and `KeyboardConfigController.DatDefaultsToChords` (`:356-366`) currently discards). ### M2 — the Camera 0x5/0x6 rows alias one `InputAction`; two independent row models, one shared live target `RetailActionIdentityTable.cs:92-104` maps ten camera actions under **both** InputMap `0x5` and `0x6` to the same `InputAction`: ```csharp foreach (uint ctx in new uint[] { 0x5, 0x6 }) { M(ctx, 0x33, InputAction.CameraMoveToward); … } ``` `KeyboardConfigController.Bind` builds one row per DAT row, so this produces 20 rows over 10 actions. Each row seeds from `bindings.CurrentForAction(action)` (`KeyboardConfigController.cs:328-330`) — the *same* list for both — and each writes through `SetForAction`, which replaces the action's entire binding set (`CloneWithout` + re-add). Three consequences: 1. **Both rows display identical chords**, not the numpad scheme in one and the arrow scheme in the other. The two `ActionKeyMapOptionRow` instances are independent, so after rebinding one, the other's model is stale and the screen shows a chord that is no longer bound. 2. **A rebind of one row silently wipes the other's**, because `SetForAction` is whole-action replacement. 3. **`FindConflict` (`:446-464`) excludes only the row being edited**, so setting a camera row to the chord its own twin displays is reported as a cross-row conflict and triggers the auto-reassign path against itself. This directly contradicts the justification the slice files for the mapping. `RetailActionIdentityRoundTripTests.cs:36-47` says the dual mapping is correct because "a user can rebind either scheme's row independently" — the mechanism makes that impossible. The register (AP-203) does not cover it either; it covers *unmapped* rows, not aliased ones. **Fix shape:** either give `InputAction` a distinct member per scheme (the honest retail shape — retail keeps them as separate `(InputMap, Action)` rows precisely because they are separately bindable), or key row storage on `(InputMapId, ActionId)` throughout and let the mapped case resolve to a per-context slice of the action's bindings rather than the whole set. The current "roughly half the rows are mapped, the rest use `RetailUnmappedKeyBindings`" split already has the per-row keyed store needed for the second option. ### M3 — the auto-reassign is silent in production, the contract asked for a prompt, and the test asserts a message production never emits Three separate problems that compound: **(a) Production wires the notification to nothing.** `RetailUiRuntime.cs:2359-2366` supplies `NotifyReassigned: _ => string.Empty`, and `DisplaySystemMessage` at `:2352-2355` skips empty strings. So `BeginSlotCapture`'s reassign branch (`KeyboardConfigController.cs:391-399`) erases another row's binding and says nothing at all. The controller's own class doc (`:96-98`) claims it "reports the outcome via `Bindings.NotifyReassigned`" — it does not. **(b) The contract asked for a prompt.** Plan §4 OP8's gate line reads "conflict prompt on a taken chord", and the contract body says "N-way cross-map conflicts + the non-user-bindable refusal per lane D §5". Retail confirms *before* overwriting (research §5.4: `OpenOverwriteBindingDialog(&conflicts)` — "confirm, may be many"). acdream already has a conflict-prompt precedent: `SettingsVM.PendingConflict` ("'X' is already bound to Y. Reassign it to Z?", research §6.1), which D1 explicitly says "feeds OP8". AP-204 itself notes `RetailDialogFactory`'s catalog is wired. So the prompt is neither unprecedented nor unbuildable here. A register row documents a deviation; it does not authorise deviating from the slice's own stated gate criterion. Per the review brief's own test — *if retail confirms before overwriting, silent reassign is a MUST-FIX-class fidelity gap, register row or not* — retail does confirm, and this is that gap. **(c) The gate script was rewritten to accept the shipped behaviour.** `docs/research/2026-08-11-campaign-op-test-script.md` §OP8 step 9 now reads "'Move Forward' silently loses its W slot … this port applies it immediately without asking first". Changing the acceptance criterion to match the implementation is a coordinator decision, not an implementer one. **(d) The test pins a behaviour production defeats.** `KeyboardConfigControllerTests.cs:88` supplies `NotifyReassigned: label => $"reassigned from {label}"` and `:240` asserts `fake.Messages` contains "reassigned". That test passes while the shipped screen is silent — false confidence exactly where the risk is. **Minimum fix:** either build the confirm dialog (contract-faithful), or — if the coordinator accepts the narrowing — supply a real notification string so the destruction is at least visible, fix the controller doc, and have the coordinator (not the implementer) amend the plan's gate line and the script together. --- ## SHOULD-FIX ### S1 — the conflict scan is first-match, and the non-bindable check is ordered after it `FindConflict` (`KeyboardConfigController.cs:446-464`) returns on the **first** matching row, and only reaches the acdream-only ("non-bindable") scan when no row matched. Retail does the opposite (research §5.4): it collects *all* conflicting `(map, control, action)` triples, and **if any one of them is non-user-bindable it refuses outright**. So a chord that conflicts with both a DAT row and an acdream-only action gets auto-reassigned here where retail would refuse; and when a chord is held by three rows, only one is cleared, leaving the other two still holding it. The class doc at `:84-94` describes this as retail's N-way model; it is a first-match model. ### S2 — the DAT's own `ActionMap.ConflictingMaps` table is never read `RetailActionMapReader.Read` (`src/AcDream.Core/Input/RetailActionMap.cs:197-221`) iterates `actionMap.InputMaps` only. The `ActionMap` DBObj also exposes `ConflictingMaps` (`Dictionary` — see the test's own construction at `RetailActionMapReaderTests.cs:47`), which is near-certainly what retail's `ICIDM::FindConflictingInputMaps` consults. The port instead treats all 306 rows as one flat conflict universe, so chords that retail considers legitimately shared across non-conflicting contexts are reported as conflicts. `RetailDefaults()` proves this is a live case: Insert / PageUp / Delete / End / PageDown are each bound in all three of `MeleeCombat`, `MissileCombat` and `MagicCombat` (`KeyBindings.cs:257-282`), and those are distinct DAT rows under contexts `0x10000003/4/5`. Rebinding any of them will trip a spurious conflict and (per M3) silently erase a sibling. ### S3 — the OK button's save has no error handling, unlike the established precedent `RetailUiRuntime.cs:2346-2351`: ```csharp Save: () => { dispatcher.Bindings.SaveToFile(keyboard.KeyBindingsFilePath); unmapped.SaveToFile(unmappedPath); }, ``` The codebase's existing writer for the same file wraps this in try/catch and logs (`src/AcDream.App/Settings/RuntimeKeyBindingTarget.cs:34-44`, whose comment explicitly reasons about not rolling back the live binding on persistence failure). Here an IO failure throws out of `UiButton.OnClick`, through `UiRoot.BubbleEvent`, into the input/render loop. Match the existing pattern. ### S4 — assigning a high slot collapses onto a low one `BeginSlotCapture` (`:408-411`) pads with `default(KeyChord)` up to the target slot, then `ReplaceSlotValue` (`:425-426`) strips every `default` before storing. So on a row with no bindings, clicking "Mapping 3" and pressing a key puts the label on **Mapping 1**; on a row with one binding, clicking "Mapping 3" lands on Mapping 2. Retail's `SetBinding(qc, slot)` writes the slot the user clicked. (The erase path's shift via `RemoveAt` is retail-consistent — `m_qclCurrent` is a list — so this is specifically the assign path.) ### S5 — ~330 DAT layout imports at startup for a screen most sessions never open `MountKeyboardConfig` is unconditional in the mount chain (`RetailUiRuntime.cs:401`) and `KeyboardConfigController.Bind` builds every header and every one of the 306 rows eagerly. Each row is one `AddItemFromTemplateList` → `TemplateResolver` call (`UiTemplateListBox.cs:172-183`), and the production resolver (`RetailUiRuntime.cs:2312-2329`) does a fresh `LayoutImporter.ImportInfos` + `LayoutImporter.Build` under `_bindings.Assets.DatLock` every time — `ImportInfos` (`LayoutImporter.cs:243-254`) has no cache. That is ~330 lock cycles, layout resolves and widget-tree builds (each row = 3 buttons + a synthesized `UiText`) on every launch. The existing Options tabs are 50 and 27 rows; this is roughly 6× the largest. Build the rows on first `Toggle` instead. ### S6 — `UiButton`'s right-click addition does change existing behaviour for disabled buttons `src/AcDream.App/UI/UiButton.cs:530-533`: ```csharp case UiEventType.RightClick: if (!Enabled) return true; OnRightClick?.Invoke(); return OnRightClick is not null; ``` Before this commit `UiEventType.RightClick` fell through to `default: return false` (`:547-548`), i.e. unhandled → `UiRoot.BubbleEvent` continued to the parent. Now a **disabled** `UiButton` returns `true` and swallows the right-click. The enabled-with-no-handler case is unchanged (`false`), so the blast is narrow, and the new shape is arguably *more* consistent with the `Click` case above it (`:521`), which already swallows when disabled. But the XML doc's claim at `:52-54` — "every pre-existing `UiButton` is unaffected … it does not change any existing click/drag behavior" — is not accurate. Either return `false` when `OnRightClick` is null regardless of `Enabled`, or correct the doc and name the disabled-swallow as intentional. --- ## NOTE **N1 — the `0x26000000` / `0x27` resolution is correct, and better-anchored than its own citation.** `RetailActionMap.cs:14-16` calls it "empirically" resolved. It is in fact hard-anchored in the decomp: `docs/research/named-retail/acclient_2013_pseudo_c.txt:34743` shows the `MasterDBMap::sm_DBTypeDefHash` registration `var_38 = 0x26000000; var_34_39 = 0x2600ffff;` with extension `".actionmap"`, and `:30960` (`0x0041c6a0`) shows the DID→type dispatch returning `0x27` for `arg2 >= 0x26000000 && arg2 <= 0x2600ffff`. So "`0x27` is the type tag, not the DID high byte; the object lives in `0x26000000..0x2600FFFF`" is verified, not merely observed. Add the citation per the project's retail-anchor rule. The narrower "single instance" claim remains probe-only. **N2 — the union-merge order-independence claim is asserted, not pinned.** `Read_UnionMergesBothMasterMapsWithoutOrderDependency` (`RetailActionMapReaderTests.cs:64-139`) builds a **synthetic** disjoint shape and asserts the union picks up both sides. It does not verify the actual premise — that the two shipped master maps' action sets under their one shared context (`0x5`) are disjoint. That premise is load-bearing: retail runs `CMasterInputMap::Merge(&m_InputMap, o, 1)` whose overlap semantics (union vs overwrite) were never traced (research §5.6), and union == overwrite only while the sets are disjoint. `Read_AgainstInstalledDats_MatchesPinnedShape` (`:209-247`) pins row counts and one spot-label but not disjointness. One extra live-DAT assertion would close it. **N3 — the six-box grouping matches lane D's retail six exactly, and the reused-id trap is handled correctly.** `KeyboardConfigController.cs:131-139` lists `0x1000049D` Movement, `0x1000049F` Camera, `0x100004A1` Combat, `0x100004A3` UI, `0x10000211` CharacterSettings, `0x100004A5` Emote — the same six containers and the same six `ActionClass` keys as research §5.1. I verified against the committed fixture that ListBox `0x10000025` and scrollbar `0x10000026` each appear **6 times**, so the per-page `UiElement.FindDescendant` scoping (`:214`, `:222`, `:230`) is both correct and necessary; a flat `layout.FindElement` would have collapsed all six pages onto one. The empirical `ActionClass` derivation (1/2/3/4/5/7, no 6) is pinned by per-class row counts in the live-DAT test (`:232-237`). **N4 — the identity byte-comparison is a permanent TEST, not a one-time check, and the three surfaced `RetailDefaults()` gaps are correctly left alone.** `RetailActionIdentityRoundTripTests.MappedActions_DatUnionDefaultBindings_MatchRetailDefaults` compares the DAT default union against `RetailDefaults()` for every mapped action, with an explicit citation-bearing allowlist (`:67-102`) for the three known disagreements — the walk-mode Shift echo, the ten `CameraAlternateControls` arrows, and the Quickslot Ctrl+N ambiguity. All three are documented in that test's class doc (`:20-60`) with reasoning, and none patches the foundational `KeyBindings.cs`. That is the right call. (The camera entry's *reasoning* is nonetheless undermined by M2.) **N5 — the "no Clear button" finding is verified.** I confirmed it directly against the committed fixture: row template `0x1000002F` (`keyboard_config_21000009.json:30050`) has `Type = 268435508 = 0x10000034` (`UIOption_ActionKeyMap`) and exactly three direct element children — `0x10000030`, `0x10000031`, `0x10000032`. No fourth Clear child. So the controller's claim at `:52-59` — that `m_buttonClear` exists in the C++ class but the shipped 2013 layout authors no affordance for it — holds against the data. **N6 — modal capture goes through the real `InputDispatcher`, and is keyboard-only.** `RetailUiRuntime.cs:2344-2345` calls `dispatcher.BeginCapture` directly, and `InputDispatcher.OnKeyDown` (`:470-488`) suppresses the action, ignores modifier-only keys, and returns `default(KeyChord)` on Escape — which the controller maps to null and treats as a silent cancel (`KeyboardConfigController.cs:385`). That is the real mechanism, correctly used. But capture always constructs `Device: 0` (`InputDispatcher.cs:483`), so a mouse chord can never be captured — meaning `CameraInstantMouseLook`'s `Device: 1` MMB-hold default can be *destroyed* through this screen (M1) but never restored through it. **N7 — the refusal text degrades to complete silence.** If `ID_KeyMapCantOverwriteReadOnlyKeymap_Label` fails to resolve, `refusalText` is null → `NonBindableRefusalText: string.Empty` (`RetailUiRuntime.cs:2357`) → `DisplaySystemMessage` skips it (`:2352-2355`). Preferring silence over invented English is the right instinct and matches the header path (`KeyboardConfigController.cs:273-277`), but combined with M3 it means *both* conflict outcomes can be invisible. Worth a diagnostic line at mount when the string does not resolve. I could not independently byte-verify the resolved value `"Could not overwrite "` without running the DAT reader; the string *key* and its `.rdata` VA `0x007BEA8C` are confirmed in research §5.4. **N8 — test coverage misses the risky seams.** Nineteen new tests, and the happy paths are well covered. Not covered: (a) activation/scope preservation — invisible by construction, since `FakeBindings.SetForAction` (`KeyboardConfigControllerTests.cs:73-77`) takes chords only, exactly like the production seam; (b) the camera dual-row alias — every test uses at most one camera row (`:102`); (c) the production capture lambda in `RetailUiRuntime` (`chord == default ? null : chord`), which is only exercised through the fake; (d) `RetailUnmappedKeyBindings.LoadOrEmpty`/`SaveToFile` round-trip — grep shows **no** test file references that type at all; (e) `RetailScanCodeMap` is exercised only transitively by the skip-if-no-DAT round-trip test. Both live-DAT tests `return` silently rather than using a real skip (`RetailActionMapReaderTests.cs:213`, `RetailActionIdentityRoundTripTests.cs:108`) — consistent with the project's `ConformanceDats` convention, but it means CI proves neither the `0x26000000` DID nor the 306-row shape. **N9 — `OnShown`/`OnHidden` are never wired.** `KeyboardConfigController` does not implement `IRetainedPanelController`, so its `OptionPage` never gets retail's visibility-driven re-seed/revert (`OptionPageModel.cs:719-746`). Rows are seeded once at mount and never re-read from live bindings. Within a session the state stays self-consistent, and `Chrome = Imported` means the window has no close affordance other than OK/Cancel, so there is no silent "close without committing" path — but it does diverge from the OnShown discipline OP4's MUST-FIX 1 established for the sibling pages. **N10 — `UiTemplateListBox`'s class doc is now stale.** It states "The Options panel's three ListBoxes (Character/Config/Chat, Campaign OP slice OP4+) are the only elements a controller will ever call `AddItemFromTemplateList` against" (`UiTemplateListBox.cs:33-35`). OP8 adds six more. **N11 — register rows AP-202/203/204 are accurate; the OP3 INERT contract is properly retired.** AP-202 (`.keymap` interchange, Load/Save-As inert), AP-203 (unmapped rows), AP-204 (silent auto-reassign + OK/Cancel left-click) each describe what the code actually does, including AP-204 stating plainly that retail shows a modal confirm first. `OptionsPanelController.cs:194-199` replaces the INERT comment with a real `BindButton`, `OpenConfigureKeyboard` is threaded from `RetailUiRuntime.cs:2062`, and the test script's §OP8 opens by stating the OP3 INERT line no longer applies. The bookkeeping is clean — the problem is M3(b)/(c), that AP-204 records a narrowing the contract did not authorise and the script was edited to match. **N12 — Escape while this screen is open quits the client.** `GameplayInputCommandController.HandleEscape` (`:260-269`) falls through to `_window.Close()`. During capture the dispatcher consumes Escape first, so the exposure is only between captures — pre-existing behaviour, not an OP8 regression, but an 800×600 screen that looks modal and whose Escape exits the game is worth the user knowing before the connected gate. **N13 — merge-readiness: the review brief's premise about `ff577641` is wrong, but nothing blocks the merge.** `ff577641` is **not** the current main-tree tip and is not an ancestor of `main`. It is the tip of `claude/latest-commits-cb0c8f` (worktree `eloquent-hugle-42119e`), the campaign branch. `main` is at `852cdda7`; the merge-base is `6bb4cfa7`, so the campaign branch is 41 commits ahead and `main` has drifted **2** commits since: `c6bc2bf7` (`tests/.../Streaming/LandblockBuildOriginTests.cs`, +16) and `852cdda7` (`docs/ISSUES.md`). Neither file is touched by OP8 — zero overlap. Within the campaign, OP8's shared-file edits are all additive and should merge cleanly: `OptionPageModel.cs` appends `ActionKeyMapOptionRow` after OP5/OP6's leaves; `RetailUiRuntime.cs` adds a record, a mount call and a mount method; `OptionsPanelController.cs` is one parameter plus one `BindButton`; `UiButton.cs` / `InputDispatcher.cs` / `WindowNames.cs` are one member each. The one thing the coordinator should watch is `OptionPageModel.cs`, which OP5, OP6 and OP8 all append to. --- ## Summary table | ID | Class | One line | |---|---|---| | M1 | MUST-FIX | `SetForAction` drops `ActivationType`/`InputScope`; Defaults collapses ~140 actions and OK persists it | | M2 | MUST-FIX | Camera ctx `0x5`/`0x6` alias one `InputAction` → 20 rows / 10 targets, stale models, mutual clobber | | M3 | MUST-FIX | Auto-reassign wired silent in production; contract asked for a prompt; gate script rewritten to accept it; test asserts a message production never emits | | S1 | SHOULD-FIX | Conflict scan is first-match; non-bindable check ordered after row match, inverting retail | | S2 | SHOULD-FIX | `ActionMap.ConflictingMaps` never read → flat conflict universe, false cross-context conflicts | | S3 | SHOULD-FIX | `Save` lacks the try/catch the existing `RuntimeKeyBindingTarget` writer uses | | S4 | SHOULD-FIX | Assigning "Mapping 3" on a sparse row lands on Mapping 1 | | S5 | SHOULD-FIX | ~330 uncached DAT layout imports at startup for a rarely-opened screen | | S6 | SHOULD-FIX | Disabled `UiButton` now swallows right-click; the "unaffected" doc claim is inaccurate | | N1–N13 | NOTE | See above — N1/N3/N4/N5/N6 are verifications that **passed**; N2/N7–N12 are gaps; N13 is merge state |