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

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<KeyChord>() 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 RowViews 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<bool>) 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