From 7133aadc6548a05b528b3f5b5a8e52baf2605bbe Mon Sep 17 00:00:00 2001 From: Erik Date: Tue, 11 Aug 2026 12:34:03 +0200 Subject: [PATCH] =?UTF-8?q?docs:=20OP8=20re-review=20=E2=80=94=20REOPEN=20?= =?UTF-8?q?(narrow):=20M1/M2/M3=20closed,=20doc+display=20residuals?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All three MUST verified mechanically: the one-pair-per-action capture claim PROVEN against RetailDefaults() (9 multi-binding actions, no counterexample); the identity map now injective (all 122 entries checked); the confirm dialog real, N-way, accept-only, with the lazy read's null unreachable in production. The OK/Cancel gesture divergence is honestly retired (no distinguishing retail affordance = no Risk symptom to write). Coordinator third-round residuals: R1 — S2's deferral is commit-message- only and M2 worsened it (0x6's DAT defaults are the arrow keys, so Defaults + the flat conflict universe makes 'Move Forward -> Up' trip a false N-way confirm; the gate script carries no warning). R2 — AP-203's row body doesn't cover the ten same-verb-sibling-live rows. SHOULD: the ten Camera Alternate rows render BLANK (retail shows the arrow keys); identity-map injectivity is load-bearing for M1+M2 with nothing enforcing it. MERGE NOTE: textually clean vs the campaign tip, but OP8's six ListBoxes sit on the #372 viewport path — blank on the branch, auto-healed by merging onto 057d8cd7+; the OP8 gate runs POST-merge. Co-Authored-By: Claude Fable 5 --- docs/research/2026-08-11-op8-rereview.md | 414 +++++++++++++++++++++++ 1 file changed, 414 insertions(+) create mode 100644 docs/research/2026-08-11-op8-rereview.md diff --git a/docs/research/2026-08-11-op8-rereview.md b/docs/research/2026-08-11-op8-rereview.md new file mode 100644 index 00000000..256625f1 --- /dev/null +++ b/docs/research/2026-08-11-op8-rereview.md @@ -0,0 +1,414 @@ +# Campaign OP slice OP8 — focused re-review of the rework + +**Reviewed:** worktree `C:\Users\erikn\source\repos\acdream\.claude\worktrees\op8-keyboard`, +branch `op8-keyboard`, rework commit `b1968ce9`, parent `b4edee97`, campaign +base `ff577641`. +**Against:** `docs/research/2026-08-11-op8-review.md` (REJECT, 3 MUST-FIX + 6 SHOULD-FIX). +**Mode:** read-only. No build, no test, no launch. Every finding is +source-derived with `file:line` evidence. + +## Verdict: **REOPEN (narrow — documentation-scoped)** + +**All three original MUST-FIX are genuinely CLOSED**, and I verified each by +walking the mechanism, not by reading the commit message. All four addressed +SHOULD-FIXes (S1/S3/S4/S6) are closed too. The rework is good work. + +Reopening is on **two residuals the M2 choice itself created** plus **the +deferral bookkeeping**, all of which are doc-scale and none of which require +re-touching the mechanism: + +- The M2 carve-out is a **new** retail deviation. The register's *section + header* names it; the **AP-203 row body does not**, and that row's "Risk" + column describes a materially different symptom. The project's register rule + ("any commit that introduces a deviation adds its register row IN THE SAME + COMMIT") is binding. +- The M2 carve-out **widens S2's deferred false-conflict surface onto the four + arrow keys** — the most commonly rebound keys on the screen — and the gate + script still carries **no S2 warning at all**. The connected gate will + produce a false defect report. + +Fixing R1+R2 is two documentation edits. R3 is one judgement call. + +--- + +## Closure check — the three MUST-FIX + +### M1 — activation/scope preservation: **CLOSED** + +The seam now carries `Binding`, not `KeyChord` +(`KeyboardConfigController.cs:181-195`; production +`RetailUiRuntime.cs:2336-2344`). Each row captures the pair once at build from +the action's first live binding, falling back to `Press`/`Game` only when the +action starts wholly unbound (`KeyboardConfigController.cs:362-367`), and +reapplies it at the single write boundary +(`KeyboardConfigController.cs:374-386`). + +**I walked the original M1 scenario end to end for a `Hold` + `MeleeCombat` +action.** `template` is a local captured by the `apply` closure, and every +write path funnels through that one closure: + +| Path | Route | Activation/Scope | +|---|---|---| +| Rebind | `ApplySlot` → `ReplaceSlotValue` → `SetCurrentValue` → `_apply` (`OptionPageModel.cs:566-571`) | preserved | +| Erase | `EraseSlot` → same | preserved | +| Defaults | `WireScreenButtons` (`:557-565`) → `Page.Defaults()` (`OptionPageModel.cs:712-717`) → `RestoreDefaultValue` → `_apply` (`:583-587`) | preserved | +| Revert / Cancel | `Page.Reset()` → `RestoreSavedValue` → `_apply` (`:577-581`) | preserved | +| OK | `Page.Apply()` → `SaveCurrentValue` (no `_apply`) then `Save()` | nothing written — nothing to lose | + +**Spot-verification of the one-pair-per-action justification against +`KeyBindings.RetailDefaults()` itself (`KeyBindings.cs:147-392`): the claim +holds.** I enumerated every action bound more than once and every action +carrying a non-default `Activation`/`Scope`: + +- Multi-binding actions: `MovementForward` (W/Up), `MovementBackup` (X/Down), + `MovementTurnLeft` (A/Left), `MovementTurnRight` (D/Right), + `MovementStrafeLeft` (Z/Alt+A/Alt+Left), `MovementStrafeRight` + (C/Alt+D/Alt+Right), `MovementWalkMode` (ShiftLeft/ShiftRight, **both** + `Hold`, `:173-174`), `CreateShortcut` (`0`/`Ctrl+0`, `:247-248`), + `CameraActivateAlternateMode` (KeypadDivide/F2, `:298-299`). Every one of + these shares a single pair across all its bindings. +- Non-default-pair actions are **all single-binding**: the 5 melee + (`:257-264`), 5 missile (`:269-273`), 13 magic + 9 `UseSpellSlot_*` + (`:274-288`), `CameraInstantMouseLook` (`:303-306`), `SelectRight` + (`:326-329`), the three `SelectDbl*` (`:333-341`), `AcdreamRmbOrbitHold` + (`:386-389`). + +**No action in `RetailDefaults()` has per-binding differing activation or +scope.** The capture-once model is sound against the production default set. +See N9 for the one shape that *can* express it (a hand-edited `keybinds.json`) +and R4 for what makes the model fragile going forward. + +The two new tests pin the two paths that mattered +(`KeyboardConfigControllerTests.cs`: `DefaultsButton_PreservesActivationAnd +Scope_ForAHoldScopedAction`, `CancelButton_PreservesActivationAndScope`), and +they assert the `Binding` fields, not just the chords — the exact gap N8 called +out as invisible-by-construction. + +### M2 — camera aliasing: **CLOSED mechanically**, with two carried consequences (R1, R2, R3) + +Only ctx `0x5` maps now (`RetailActionIdentityTable.cs:91-121`); the `foreach +(uint ctx in new uint[] { 0x5, 0x6 })` loop is gone. + +- **No dual rows over one action.** I checked the *whole* map for remaining + many-to-one entries, not just camera: 122 `M(...)` calls, every + `InputAction.*` value appearing exactly once. The map is now injective, so + the three consequences the original M2 listed (identical stale chords, mutual + clobber, self-conflict) are all structurally gone. +- **The `0x6` rows genuinely take the store-only path end to end.** + `TryResolve` misses → `mapped == false` → + `initial = CurrentForUnmapped((0x6, actionId))` and writes go to + `SetForUnmapped` (`KeyboardConfigController.cs:354-386`), which lands in + `RetailUnmappedKeyBindings` and the sibling `keybinds-unmapped.json`. The new + test asserts exactly this both ways + (`CameraContext5And6_AreIndependentRows_NotAliased`). +- **The retired allowlist entries are correct.** Ten camera entries removed + from `KnownRetailDefaultsDisagreements` + (`RetailActionIdentityRoundTripTests.cs:62-72`), with the class doc rewritten + from "three disagreements" to "two". With ctx `0x6` no longer in the union, + ctx `0x5`'s twelve camera actions match `RetailDefaults()` exactly — that + follows directly from the `RetailDefaults()` camera block (`:298-317`), which + is purely the numpad scheme. + +**On the honesty question the brief asks.** Store-only-but-bindable is a +*weaker* deception here than for the other AP-203 rows, and a *stronger* one: + +- Weaker, because the two schemes land under **different headers** — + `ID_InputMap_CameraControls` and `ID_InputMap_CameraAlternateControls` + (`RetailActionMap.cs:144-145`) — so the user is not looking at two + identically-framed rows. +- Stronger, because unlike the emote and CharacterSettings rows AP-203's Risk + column describes, **a live consumer for these verbs exists and works** — + the sibling ctx-`0x5` row on the same page drives it. "Rotate Camera Left" + works in one group and is inert in the other. That is a different symptom + from "this row has no consumer anywhere." + +So: **AP-203's wording does not cover these ten.** See R2. + +### M3 — conflict dialog: **CLOSED** + +`ConfirmOverwrite` is wired to the real `RetailDialogFactory.MakeConfirmation` +seam (`RetailUiRuntime.cs:2374-2388`), the same one +`GameplayConfirmationController` uses (`:2531-2533`). Reassign happens **only** +on accept, and the N-way collection is applied whole +(`KeyboardConfigController.cs:450-468`). Decline returns without touching a +single row — pinned by +`Capture_ConflictWithAnotherRow_DeclineLeavesBothRowsUnchanged`. AP-204 is +struck through and given a retirement note (register `:189`); gate script step 9 +is reverted to the confirm-prompt behaviour and now also exercises the decline +path (`2026-08-11-campaign-op-test-script.md:913-921`). + +**The lazy read cannot NPE.** `RetailUiRuntime.cs:2383` guards +`if (DialogFactory is null) { onResult(false); return; }` before the call. + +**Is a null read reachable?** Two cases, both handled: + +1. *Ordering.* `MountKeyboardConfig()` at `Initialize():401` runs before + `MountDialogFactory()` at `:404`. But the closure is only invoked from + `BeginSlotCapture`'s capture callback, which needs a user keypress on a + window mounted `Visible = false` (`:2408`) and opened via a toolbar/Options + path that requires `Initialize()` to have returned. Not reachable. +2. *Degradation.* If `RetailDataIdResolver.Resolve(dats, 2, 5)` returns 0, + `MountDialogFactory` logs and returns with `DialogFactory` left null + (`:2506-2510`). Then every conflict silently declines forever. Not a crash, + but see N3 — that state has no diagnostic at the capture site. + +**On the OK/Cancel left-click vs right-click-release retirement claim.** The +rework moved it from a register row to a code comment +(`KeyboardConfigController.cs:575-579`) on the argument that it is +zero-observable. **I agree, and the register's own convention supports it.** +Retail's `idMessage 0x19` right-click-release on this button pair carries no +distinguishing visual affordance — there is no cursor change, no hover cue, no +label difference that would let a user *see* which gesture the button wants. +The only observable would be "right-click does nothing here" — and after S6, +right-click on a handler-less `UiButton` bubbles to the parent exactly as it +did before OP8, so nothing surprising happens either. The register's Risk +column is defined as "the symptom you'd observe"; there is no symptom to write. +This one does not need the row the original review implied. + +--- + +## SHOULD-FIX verification + +| ID | Status | Evidence | +|---|---|---| +| **S1** | **CLOSED** | `FindConflicts` (`KeyboardConfigController.cs:525-545`) runs the non-user-bindable scan first and returns immediately (`:527-535`), then collects **every** matching row into a list (`:537-543`) instead of returning first-match. `BeginSlotCapture`'s switch (`:437-473`) honours the ordering, and the N-way set is both named in the dialog message (`:454-457`) and cleared in full on accept (`:461-465`). Two tests pin it, including the both-conflicts-exist ordering case (`Capture_ConflictWithBothARowAndANonBindableAction_NonBindableWins`). | +| **S3** | **CLOSED** | `RetailUiRuntime.cs:2351-2367` wraps both writes in try/catch + `Console.WriteLine`, matching `RuntimeKeyBindingTarget.Apply`'s discipline (and its reasoning about not rolling back the live binding). The IO throw no longer escapes `UiButton.OnClick` into `UiRoot.BubbleEvent`. | +| **S4** | **CLOSED (in-session)** | `ReplaceSlotValue` (`:496-509`) now trims only *trailing* empties via a `lastReal` scan, so `ApplySlot`'s padding survives and "Mapping 3" on an empty row lands on display index 2. `EraseSlot` (`:486-494`) early-returns on an already-empty slot instead of `RemoveAt`-shifting later bindings. `apply` filters interior `default(KeyChord)` at the write boundary (`:379`), so no padding leaks into a persisted `Binding`. Both pinned by tests. See N5 for the one thing S4 cannot fix. | +| **S6** | **CLOSED** | `UiButton.cs:530-543` now returns `false` (unhandled → bubbles) when `OnRightClick` is null regardless of `Enabled`, so the disabled-swallow the original review found is gone and the property's own doc claim (`:52-58`, "every pre-existing `UiButton` is unaffected … it does not change any existing click/drag behavior") is now **accurate**. OP8's key buttons always set the handler, so the erase path is unaffected. | +| **S2** | **DEFERRED — not honestly recorded** | See R1. `ConflictingMaps` appears nowhere in the branch except `RetailActionMapReaderTests.cs` (as a test-fixture construction detail) and the commit message. No register row, no `docs/ISSUES.md` entry, no plan-ledger note, no gate-script warning. | +| **S5** | **DEFERRED — not recorded** | See R5. `MountKeyboardConfig` is still unconditional at `RetailUiRuntime.cs:401`; nothing durable records the deferral. | + +--- + +## Residuals + +### MUST-FIX + +#### R1 — S2's deferral is recorded only in a commit message, and the M2 carve-out just made its blast radius worse. The gate script does not warn. + +The conflict scan is a flat universe over all 306 rows +(`KeyboardConfigController.cs:537-543` iterates `_rows` with no InputMap +filter). Two live consequences at the OP8 connected gate: + +1. **Shared combat keys (pre-existing, called out by the original S2).** + Insert / PageUp / Delete / End / PageDown are each bound in all three of + `MeleeCombat`, `MissileCombat` and `MagicCombat` by design + (`KeyBindings.cs:257-282`), under distinct DAT contexts + `0x10000003/4/5`. Rebinding any of them now opens a confirm naming two + sibling rows retail would never consider conflicting — and **accepting + erases both**, where before M3 it silently erased one. M3 makes it visible + and declinable, which is strictly better, but it is still a false conflict. +2. **Arrow keys (NEW — introduced by the M2 carve-out).** ctx `0x6`'s DAT + defaults are the arrow cluster (the rework's own test seeds + `RetailKeyChord(0xCB, …)` = `DIK_LEFT` for `(0x6, 0x35)`; + `KeyboardConfigControllerTests.cs`). Before M2 those rows displayed the + *mapped* action's numpad chords, so they never claimed an arrow. After M2 + they are unmapped, and **clicking Defaults writes the arrows into the + unmapped store** (`RestoreDefaultValue` → `apply` → + `SetForUnmapped`). From that point Left/Right/Up/Down are held by both the + Movement rows (`KeyBindings.cs:153-165`) *and* ten Camera Alternate rows — + so rebinding "Move Forward" to Up-arrow trips an N-way confirm naming a + camera row, and accepting erases it. Gate script step 12 explicitly + instructs the user to click Defaults + (`2026-08-11-campaign-op-test-script.md:987-990`). + +The gate script's "What to report" section (`:983-1001`) has no S2 line. The +user will file this as a defect. + +**Fix (docs only):** (a) a register row for the flat-conflict-universe +divergence — retail consults `ActionMap.ConflictingMaps` / +`ICIDM::FindConflictingInputMaps`, we do not — or a `docs/ISSUES.md` entry +referenced from the plan ledger; and (b) an explicit warning in the gate +script's §OP8 "What to report", naming the combat cluster and the arrow keys as +*known* false conflicts so the user reports the *wording*, not the *existence*. + +#### R2 — the M2 carve-out is a new deviation whose register row does not name it + +`retail-divergence-register.md:178` (the AP section header) was amended to say +"all 10 CameraAlternateControls rows per the M2 de-alias fix". **The AP-203 row +itself (`:188`) was not.** Its body enumerates "82 of 87 Emote rows", "all 48 +CharacterSettings hotkey rows", "Spell Slot 10-12, Quickslot 10-13", and +"roughly twenty UI-panel-toggle rows" — no camera. Its Risk column reads: + +> A user rebinds e.g. an emote or a CharacterSettings hotkey … and the binding +> persists but has no observable in-game effect + +That is not this symptom. For the camera ten, the verb **does** have a live +consumer that **does** work — through the sibling row in the group directly +above it on the same page. "Two rows, same page, same verb, one live one inert" +is a distinct observable and belongs in the Risk column, per the register's own +rule that the column is written as the symptom you would observe. + +**Fix (docs only):** extend AP-203's row body + Risk column to name the ten +`(0x6, …)` rows and their distinct symptom, or file a dedicated AP row for the +carve-out. The section header alone does not satisfy the same-commit rule. + +### SHOULD-FIX + +#### R3 — the ten Camera Alternate rows render **blank**; retail shows the arrow keys + +`BuildActionRow` seeds an unmapped row's display from the store only +(`KeyboardConfigController.cs:369-371`), and `RetailUnmappedKeyBindings.Get` +returns `Array.Empty()` for an absent key +(`RetailUnmappedKeyBindings.cs:23-26`) — nothing anywhere seeds the store from +`row.DefaultBindings`. On a fresh install the "Camera Alternate Controls" group +shows ten rows with all three Mapping buttons blank. The rework's own test pins +this as intended (`Assert.Empty(ctx6.Model.Current)` and +`Assert.Empty(fake.Unmapped)`), and `RetailActionMapReaderTests.cs:233` +(`Assert.Equal(22, byClass[RetailActionClass.Camera])`) confirms all 22 rows do +render. + +This is a *new* visual regression: before M2 those rows displayed chords (the +wrong ones, but chords). + +**Watch the fix.** Naively seeding the display from DAT defaults creates an +erase-idempotency trap: erase → store empty → next open re-seeds from DAT → +the erase silently un-does itself. Any seeding needs a written-through +"materialised defaults" store entry, or an explicit empty marker. If the +coordinator judges that out of scope, this needs to be R1's third gate-script +warning line, because ten blank rows on the Camera tab will be reported. + +#### R4 — the identity map's injectivity is now load-bearing and nothing enforces it + +M1's capture-once model and M2's de-alias both assume **exactly one DAT row per +`InputAction`**. Break that assumption and the failure is silent: two rows +would again share one live target, each with its own captured +`(Activation, Scope)`, and the last writer wins. I verified injectivity by hand +over all 122 entries in `RetailActionIdentityTable.cs:70-253` — it holds today — +but there is no `Debug.Assert`, no `Bind`-time duplicate log, and no test. +`RetailActionIdentityRoundTripTests` checks DAT-default agreement, not +uniqueness of values. + +**Fix:** one test asserting `Map.Values.Distinct().Count() == Map.Count`, or a +`Bind`-time warning when two `RowView`s resolve to the same `MappedAction`. + +#### R5 — S5's deferral has no durable record either + +`MountKeyboardConfig` is still unconditional (`RetailUiRuntime.cs:401`) and +`Bind` still builds all 306 rows + 19 headers eagerly, each through the +uncached `LayoutImporter.ImportInfos` path under `DatLock` +(`:2316-2330`). The deferral lives only in the commit message. Same fix shape +as R1(a): an `ISSUES.md` entry or a plan-ledger line. + +#### R6 — the plan ledger's OP8 row is stale + +`docs/plans/2026-08-10-options-panel-campaign.md:427` reads +`| OP8 | CODE-COMPLETE, gate READY | (this branch's commit) | not yet reviewed | …`. +Every sibling row carries real SHAs and a review chain (see OP5/OP6/OP7 at +`:424-426`). This is the coordinator's row to fill, but it is currently the +only durable place the S2/S5 deferrals could have been recorded and was not. + +--- + +## NOTE + +**N1 — merge overlap: textually clean, but OP8 has a hard *semantic* dependency +on `057d8cd7`.** The campaign branch gained three commits since `ff577641`: +`1e36d4a7` (the OP8 review doc), `c3ed32fb` and `057d8cd7` (#372). Changed +files: `docs/ISSUES.md`, `docs/research/2026-08-11-op8-review.md`, +`src/AcDream.App/UI/UiTemplateListBox.cs`, +`tests/…/OptionsPanelLiveMountProbeTests.cs`, +`tests/…/UiTemplateListBoxViewportTests.cs`. **OP8 touches none of them** — a +read-only `git merge-tree` over the merge-base reports zero conflicts. + +But `057d8cd7` is the #372 fix: `UiTemplateListBox`'s lazily-created viewport +was constructed 0×0 with fill-anchors, which captured a degenerate anchor +baseline and made `UiScrollablePanel.LayoutScrollableChildren` cull every row — +so every ListBox-backed Options tab rendered **blank**. OP8 adds **six more** +`UiTemplateListBox` instances driven through the identical lazy-viewport path +(`KeyboardConfigController.cs:251-260`). **On `b1968ce9` as it stands, all six +Configure Keyboard pages will render blank.** The branch must be merged onto +(or rebased over) a campaign tip at or after `057d8cd7` before the OP8 +connected gate, or the gate fails for a reason that has nothing to do with OP8. + +**N2 — `ShowConfirmation` already existed with the exact shape the M3 wiring +needed.** `RetailUiRuntime.cs:588-595` is +`ShowConfirmation(string, Action)` doing precisely +`DialogFactory?.MakeConfirmation(msg, data => completed(data.GetBoolean( +RetailDialogProperty.ConfirmationResult)))`. The rework inlined a near-duplicate +at `:2380-2387`. The only behavioural difference is the null-factory case (the +inline version calls `onResult(false)`; `ShowConfirmation` returns `0u` and +never calls back — same net effect, no reassign). Worth collapsing. + +**N3 — the degraded-DialogFactory state is silent at the capture site.** If the +dialog catalog fails to resolve, `MountDialogFactory` logs once at startup +(`:2506-2510`) and every subsequent conflict declines with no user-visible +feedback. Combined with the original N7 (the refusal string degrading to +silence via `NonBindableRefusalText: refusalText ?? string.Empty` at `:2373`), +**both** conflict outcomes can be invisible in that state. One diagnostic line +in the `DialogFactory is null` branch would close it. + +**N4 — the confirm goes on the default dialog queue, and it is modal.** +`MakeConfirmation` uses `DefaultQueueKey` (`RetailDialogFactory.cs:125-136`), +so a gameplay confirmation already open queues the rebind confirm behind it. +That is fine because `CreateDialog` sets `_host.Modal = view.Root` +(`RetailDialogFactory.cs:279`) — which closes the reentrancy hole I went +looking for: the user cannot click OK/Cancel on the keyboard screen, or start a +second capture, while a rebind confirm is pending, so the deferred +`ApplySlot` can never land after the screen was cancelled or saved. + +**N5 — S4's slot position is session-only.** `KeyBindings` has no slot concept, +so a chord placed on "Mapping 3" of an empty row persists as a single binding +and re-displays on "Mapping 1" after OK + relaunch (`BuildActionRow`'s +`initial` is a dense `Select(b => b.Chord)`, `:369-370`). Retail's own +`m_qclCurrent` is also a list, so this may match retail; either way it is +beyond what S4 asked for and worth knowing before the gate. + +**N6 — an action with retail defaults cannot be persistently unbound, and OP8 +is the first UI that exposes erase.** `KeyBindings.LoadOrDefault`'s +merge-over-defaults (`KeyBindings.cs:469-477`) re-adds `RetailDefaults()` +bindings for any action the loaded file has none for. So: right-click-erase all +three slots of "Move Forward", click OK, relaunch — W and Up are back. That is +pre-existing `KeyBindings` behaviour, not an OP8 defect, but gate script steps +11 and 14-16 walk the user right into it. + +**N7 — `FindConflicts`' non-bindable scan is quadratic per capture.** +`KeyboardConfigController.cs:529-534` iterates every `InputAction`, runs +`RetailActionIdentityTable.Map.Values.Contains(candidate)` (a linear scan over +122 values) for each, and calls `CurrentForAction` — which in production is +`dispatcher.Bindings.ForAction(action).ToArray()`, allocating an array per +action. That is ~250 array allocations and ~30k comparisons per keypress in a +config screen: harmless, but a `HashSet` built once would be free. + +**N8 — chords held by real-but-unmapped retail actions are hard-refused.** With +S1's reordering, any chord bound to an `InputAction` absent from the identity +table now refuses outright before the row scan. Two of those are *not* +acdream-only debug keys: `SelectionPickUp` (**F**) and +`ScrollUp`/`ScrollDown` (**Ctrl+Up** / **Ctrl+Down**) — all bound in +`RetailDefaults()` (`:184`, `:345-346`) but absent from the identity map's +`0x10000007` and `0x10000009` blocks. So the user cannot bind anything to F and +gets "Could not overwrite " with no explanation. Pre-existing (`b4edee97`), not +a rework regression, and arguably the honest behaviour — but it is a plausible +gate report. + +**N9 — the capture-once model's one expressible counterexample.** +`keybinds.json` carries `activation` and `scope` **per binding** +(`KeyBindings.cs:433-459`), so a hand-edited file *can* give one action two +bindings with differing pairs; this row model would collapse them to the first +binding's pair on the next write. No acdream code path produces that shape, and +`SaveToFile` (`:499-519`) round-trips whatever it is given, so the exposure is +limited to manual editing. Worth one sentence in the class doc's M1 paragraph +(`KeyboardConfigController.cs:84-105`), which currently states the invariant as +a property of `RetailDefaults()` without noting the file format is wider. + +--- + +## Summary table + +| ID | Class | One line | +|---|---|---| +| M1 | **CLOSED** | Full `Binding` seam; pair captured once and reapplied on rebind/Defaults/Cancel/Revert; one-pair-per-action verified against `RetailDefaults()` — no counterexample | +| M2 | **CLOSED** | ctx `0x6` de-mapped; map now injective (122/122 verified); `0x6` rows take the store-only path end to end; ten allowlist entries correctly retired | +| M3 | **CLOSED** | Real `MakeConfirmation` confirm before reassign, N-way, accept-only; lazy read cannot NPE and is unreachable-null in production; the OK/Cancel gesture note genuinely does not warrant a register row | +| S1 | **CLOSED** | Non-bindable checked first; all conflicting rows collected | +| S3 | **CLOSED** | `Save` try/catch matching `RuntimeKeyBindingTarget` | +| S4 | **CLOSED** | Trailing-only trim; empty-slot right-click is a no-op | +| S6 | **CLOSED** | `false` when no handler regardless of `Enabled`; doc claim now accurate | +| R1 | MUST-FIX | S2 deferral recorded only in a commit message; M2 added the arrow keys to the false-conflict surface; gate script has no warning → false defect at the gate | +| R2 | MUST-FIX | M2's carve-out is a new deviation; AP-203's *row* does not name it and its Risk column describes a different symptom (section header alone ≠ the same-commit rule) | +| R3 | SHOULD-FIX | Ten Camera Alternate rows render blank vs retail's arrow keys; no DAT-default seeding for unmapped rows (watch the erase-idempotency trap in any fix) | +| R4 | SHOULD-FIX | Identity-map injectivity is now load-bearing for M1 *and* M2 but unenforced — no assert, no test | +| R5 | SHOULD-FIX | S5 deferral has no durable record either | +| R6 | SHOULD-FIX | Plan ledger's OP8 row still says "not yet reviewed" / "(this branch's commit)" | +| N1 | NOTE | Merge is textually clean (zero overlap, `merge-tree` conflict-free) but OP8 **depends on** `057d8cd7` (#372) — without it all six keyboard pages render blank | +| N2–N9 | NOTE | Duplicate `ShowConfirmation`; silent degraded DialogFactory; queued-but-modal confirm (no reentrancy hole); session-only slot position; erase can't persist over `LoadOrDefault`'s merge; quadratic non-bindable scan; F / Ctrl+Arrow hard-refused; per-binding pairs expressible in JSON |