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

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:

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<InputAction, IReadOnlyList<KeyChord>>, 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:

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<uint, InputsConflictsValue> — 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:

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:

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