From 1e36d4a7a1adba5504313d75c6973fd6b2389689 Mon Sep 17 00:00:00 2001 From: Erik Date: Tue, 11 Aug 2026 09:35:13 +0200 Subject: [PATCH] =?UTF-8?q?docs:=20OP8=20combined=20review=20=E2=80=94=20R?= =?UTF-8?q?EJECT=20(3=20MUST):=20activation-loss,=20camera=20aliasing,=20s?= =?UTF-8?q?ilent=20reassign?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit M1: SetForAction rebuilds bindings as new Binding(chord, action), defaulting Press/InputScope.Game and dropping the read chord's ActivationType/InputScope — MovementWalkMode (Hold), the three melee heights (Hold+MeleeCombat), Missile/Magic-scoped rows, and CameraInstantMouseLook (Hold, Device 1) all collapse; RestoreDefaultValue invokes _apply so ONE Defaults click flattens ~140 actions and OK persists it (gate step 12 tells the user to click Defaults). M2: the ten camera 0x5/0x6 rows alias one InputAction each — 20 rows over 10 targets, twins seed identically, clobber each other, self-conflict; the round-trip test justifies the dual map on independence the mechanism makes impossible. M3: silent auto-reassign (NotifyReassigned -> empty, DisplaySystemMessage skips empty) destroys a binding with zero feedback while the doc claims it reports; retail confirms first (OpenOverwriteBindingDialog), the plan gate says 'conflict prompt', the tested SettingsVM prompt precedent exists, AP-204 concedes RetailDialogFactory is wired, and a fake asserts a message production contradicts. SHOULD: non-bindable check ordered after first-match conflict (inverts retail's any-non-bindable-refuses); ActionMap. ConflictingMaps never read (false conflicts on legitimately-shared combat keys); Save lacks the writer's try/catch; sparse-row Mapping-3 lands on Mapping-1; ~330 uncached layout imports at startup. Clean: the 0x26000000 DID (better-anchored than cited — .actionmap MasterDBMap range, 0x27 is the type tag); no-Clear-button confirmed against the fixture; six-box grouping matches lane D; identity byte-comparison is a permanent test; modal capture real; AP-202/203/204 accurate; OP3 INERT retired. Brief-premise correction: ff577641 is the CAMPAIGN tip (claude/latest-commits-cb0c8f), not main's (852cdda7); OP8 additive, only OptionPageModel.cs is a triple-append merge watch-point. Rework round 1 to the isolated op8-keyboard worktree. Co-Authored-By: Claude Fable 5 --- docs/research/2026-08-11-op8-review.md | 448 +++++++++++++++++++++++++ 1 file changed, 448 insertions(+) create mode 100644 docs/research/2026-08-11-op8-review.md diff --git a/docs/research/2026-08-11-op8-review.md b/docs/research/2026-08-11-op8-review.md new file mode 100644 index 00000000..662deac1 --- /dev/null +++ b/docs/research/2026-08-11-op8-review.md @@ -0,0 +1,448 @@ +# 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 |