diff --git a/docs/research/2026-08-11-op4-review-blast.md b/docs/research/2026-08-11-op4-review-blast.md new file mode 100644 index 00000000..d07cc8b0 --- /dev/null +++ b/docs/research/2026-08-11-op4-review-blast.md @@ -0,0 +1,399 @@ +# OP4 review — regression / blast-radius lens + +**Commit under review:** `22b86b9f` "feat(ui): Campaign OP slice OP4 — the Character tab" +**Reviewer lens:** regression + blast radius (both axes on every changed surface) +**Method:** read-only. Diff walk, pre/post consumer enumeration by `git grep` at +`22b86b9f^` and `22b86b9f`, fixture-tree analysis of +`options_panel_2100006E_1000018D.json`, decomp cross-check against +`docs/research/named-retail/acclient_2013_pseudo_c.txt` and `acclient.h`. +No build, no test run, no launch. + +**Verdict: APPROVE-WITH-FIXES.** M1–M3 must land before the OP4 connected gate +is run — the gate script this commit ships (`docs/research/2026-08-11-campaign-op-test-script.md` +§OP4 steps 3 and 13) contains steps that cannot pass against this code, so +running it now burns a user session on a known-failing check. + +--- + +## 1. Group-C re-point: per-option old-consumer enumeration + +Seven ids moved from the client-local `GameplaySettings` record (settings.json) +to `RuntimeCharacterOptionsState.GetOptionBit`. For each: every read site of the +OLD value at `22b86b9f^`, and whether it moved. + +### AutoTarget (`0x0D`, Options1 `0x2000`, batched, ClientDefault **ON**) + +| Old read site (`22b86b9f^`) | Moved? | +|---|---| +| `Composition/FrameRootComposition.cs:599` → `CombatCameraTargetSource(d.Settings, …)` | ✅ → `CharacterOptionCombatSettingsSource` | +| `Composition/SessionPlayerComposition.cs:770` → `LiveCombatAttackOperations(… d.Settings …)` | ✅ | +| `Composition/InteractionRetainedUiComposition.cs:286` → `LiveCombatTargetOperations(autoTarget: …)` | ✅ | +| `UI/Layout/CombatUiController.cs:116` (LED click writes) / `:193` (LED seed reads) | ❌ **NOT moved** | +| `Settings/RuntimeSettingsController.cs:236` `public bool AutoTarget => Gameplay.AutoTarget` | ❌ now dead (no consumer) | +| `Settings/RuntimeSettingsController.cs:422,433` (`SetCombatGameplay` VM mirrors) | ❌ still mirrors the orphan | +| `Headless/Hosting/HeadlessGameplayOperations.cs:127` `AutoTarget => true` (hardcoded) | ❌ never re-pointed | + +### AutoRepeatAttack (`0x00`, Options1 `0x02`, **auto-save**, ClientDefault ON) + +| Old read site | Moved? | +|---|---| +| `Composition/SessionPlayerComposition.cs:770` → `LiveCombatAttackOperations` (→ `RuntimeCombatAttackState:380,403,411`) | ✅ | +| `UI/Layout/CombatUiController.cs:114` / `:192` | ❌ **NOT moved** | +| `Settings/RuntimeSettingsController.cs:238` | ❌ now dead | +| `Headless/Hosting/HeadlessGameplayOperations.cs:126` `AutoRepeatAttack => false` | ❌ never re-pointed | + +### ViewCombatTarget (`0x07`, Options1 `0x80`, batched, ClientDefault **OFF**) + +| Old read site | Moved? | +|---|---| +| `Combat/CombatCameraTargetSource.cs:44` (via injected source) | ✅ | +| `UI/Layout/CombatUiController.cs:118` / `:194` | ❌ **NOT moved** | +| `Settings/RuntimeSettingsController.cs:240` | ❌ now dead | + +⚠️ **Observable-default flip.** `GameplaySettings.Default.ViewCombatTarget = true` +(`src/AcDream.UI.Abstractions/Panels/Settings/GameplaySettings.cs:51`) but bit +`0x80` is CLEAR in retail's `CharacterOptions1.Default = 0x50C4A54A` +(→ `CharacterOptionTable.cs:118` `ClientDefault: false`). A fresh install's +combat camera stops tracking the target by default. No register row filed. + +### VividTargetingIndicator (`0x0E`, Options1 `0x8000`, batched, ClientDefault ON) + +| Old read site | Moved? | +|---|---| +| `Composition/InteractionRetainedUiComposition.cs:676` → `VividTargetRuntimeBindings` | ✅ (polled lambda) | +| `Panels/Settings/SettingsPanel.cs:322` | n/a — `SettingsPanel` has **no production construction site** (`new SettingsPanel(` appears only in `tests/AcDream.UI.Abstractions.Tests/…:40`) | + +### CoordinatesOnRadar (`0x14`, Options1 `0x400000`, batched, ClientDefault ON) + +| Old read site | Moved? | +|---|---| +| `Composition/LivePresentationComposition.cs:836` → radar snapshot | ✅ (polled lambda) | + +### LockUI (`0x33`, Options2 `0x1000000`, **auto-save**, ClientDefault OFF) + +| Old read site | Moved? | +|---|---| +| `Composition/LivePresentationComposition.cs:837` `uiLocked:` (radar) | ✅ polled — converges live | +| `Composition/InteractionRetainedUiComposition.cs:477` `host.Root.UiLocked = …` | ⚠️ moved but **one-shot at mount**, never re-read → does NOT converge | +| `Net/LiveSessionRuntimeFactory.cs:387` `ToggleUiLock` read | ✅ reads the bit — but see **M3** | +| `Settings/RuntimeSettingsController.cs:355,359,362,369` (`SetUiLocked` writer) | ❌ still writes the orphan, and is still called from `ToggleUiLock` | + +Note: `LockUI` is deliberately NOT a Character-tab row +(`CharacterOptionsPageControllerTests.ExcludedIds_HaveNoRow`), and the OP4 gate +script's Group-C section (items 17–18) does not list it — so this re-point ships +ungated. + +### AcceptLootPermits (`0x10`, Options1 `0x80000`, **auto-save**, ClientDefault OFF) + +| Old read site | Moved? | +|---|---| +| `Net/LiveSessionRuntimeFactory.cs:452` (`ClientCommandController.Bindings.AcceptLootPermits`) | ✅ | +| `Net/LiveSessionRuntimeFactory.cs:454` (`SetAcceptLootPermits`) | ✅ (now sends 0x0005 **and** writes the orphan) | +| `UI/ClientCommandController.cs:444,450` (`/consent on|off`) | ✅ via the bindings above | +| `Net/LiveSessionCommandRouter.cs:458,459` | ✅ pure forwarders | + +### ToggleRun (`0x0A`, Options1 `0x400`, batched, ClientDefault ON) — Group B + +| Old read site | Moved? | +|---|---| +| `Input/DispatcherMovementInputSource.cs:77` hardcoded `!walking` | ✅ → `RunAsDefaultMovement != walking` | +| `Panels/Settings/SettingsPanel.cs:314` | n/a (test-only surface) | + +--- + +## 2. MUST-FIX + +### M1 — The Character tab is seeded before login and never re-seeded; Reset then writes the stale baseline over server truth + +`RetailUiRuntime.Initialize()` (`src/AcDream.App/UI/RetailUiRuntime.cs:371`) +calls `MountOptionsPanel()`, which calls +`CharacterOptionsPageController.Bind` (`RetailUiRuntime.cs:2010`). That whole +chain runs inside `lease.Mount(…)` +(`src/AcDream.App/Composition/InteractionRetainedUiComposition.cs:876`), which +runs inside `InteractionRetainedUiCompositionPhase` — a **startup** phase of +`GameWindow.OnLoad` (`src/AcDream.App/Rendering/GameWindow.cs:1231`, chain at +`:1325-1369`). No network session exists yet. + +So at `CharacterOptionsPageController.cs:368`: + +```csharp +bool initial = bindings.CurrentValue(spec.Id); // == GetOptionBit(...) +checkbox.Selected = initial; +var row_ = new BoolOptionRow(initial, entry.ClientDefault, apply: …); +``` + +`initial` is read from `RuntimeCharacterOptionsState`'s **constructor** words +(`RuntimeCharacterState.cs:671-672`, `DefaultOptions1 = 0x50C4A54A` / +`DefaultOptions2 = 0x00948700`), not the character's server options. +`PlayerDescription` installs the real words later at +`src/AcDream.Runtime/Session/LiveSessionEventRouter.cs:223` (`Options.Replace`), +and the only subscriber to `OnCharacterOptionsChanged` is +`SyncChatFromServerOptions` (`Net/LiveSessionRuntimeFactory.cs:323-324`). +**Nothing re-seeds the 50 rows.** Consequences: + +1. Gate-script step 3 ("Confirm the checkboxes reflect your character's actual + state … seeded from the live server bit at panel-mount time") is false for + every option whose server value differs from the client constructor default. +2. Worse — the `BoolOptionRow` `_saved` baseline is also stale. Take a batched + option the server has ON but whose constructor default is OFF (e.g. + `SideBySideVitals`). Row seeds `_current = _saved = false`. User clicks it → + `SetCurrentValue(true)` → `TrySetOption` sees the bit already true → no-op + (correct). User changes their mind and clicks **Reset** (or just switches + tab, which is `OnHidden() → Reset()`, `OptionPageModel.cs:261`) → + `Changed == true` → `RestoreSavedValue()` → `_apply(false)` → + `SetSingleCharacterOptionRuntimeCmd(id, false)` → **the server bit is turned + OFF.** A visually-idempotent "toggle then cancel" mutates real character + state in the wrong direction. This is the "contract asserting a mechanism + that does not exist" class the C4 closeout handoff flags as binding. +3. `host.Root.UiLocked = d.Character.Options.GetOptionBit(LockUI)` + (`InteractionRetainedUiComposition.cs:477-486`) has the same shape but is a + one-shot assignment, so retained-window locking never converges to the + server bit — while the radar's `uiLocked` lambda + (`LivePresentationComposition.cs:843-844`) polls and does converge. Two + consumers of one option, permanently able to disagree. + +The fix shape is a re-seed hook on `OnCharacterOptionsChanged` (the seam CH3 +already established for chat) that re-runs each row's +`SetSeed(current)` — i.e. rewrite `_current`, `_saved`, and `checkbox.Selected` +without invoking `_apply`. That verb does not exist on `BoolOptionRow` today. + +### M2 — Two writable copies: the Combat panel's three LEDs still drive the orphaned `GameplaySettings` + +`src/AcDream.App/UI/Layout/CombatUiController.cs:113-118` writes and `:192-194` +reads `GameplaySettings.AutoRepeatAttack / AutoTarget / ViewCombatTarget`, +wired in production at `InteractionRetainedUiComposition.cs:644-648` +(`() => d.Settings.Gameplay`, `d.Settings.SetCombatGameplay`) and mounted at +`RetailUiRuntime.cs:366` (`MountCombat()`). Nothing gameplay-facing reads that +record any more — every behavioral consumer now reads the server bit. Net +effect: + +- The retail Combat window's **Repeat Attacks / Auto Target / Keep in View** + checkboxes are now decorative: clicking them changes combat behavior not at + all, and never reaches the wire. +- They and the Character tab's rows for the *same three retail + `PlayerOption`s* read from different stores, so they will visibly disagree. +- `RuntimeSettingsController.SetCombatGameplay` (`:415-442`) keeps persisting + the divergent copy to settings.json, so the disagreement survives restarts. + +This is exactly the "two writable copies" failure mode the CH3 research warned +about, and it is the one old read site of the three combat options that the +re-point missed. The Combat panel's LEDs must route through +`SetSingleCharacterOptionRuntimeCmd` and seed from `GetOptionBit`, in this +slice or an immediately-following one. + +### M3 — `ToggleUiLock`'s new dual write can silently drop the visual lock + +`src/AcDream.App/Net/LiveSessionRuntimeFactory.cs:392-400`: + +```csharp +bool locked = !_domain.Character.Options.GetOptionBit(CharacterOptionId.LockUI); +SendSingleCharacterOption((uint)CharacterOptionId.LockUI, locked); +_interaction.Settings.SetUiLocked(locked); +``` + +`RuntimeSettingsController.SetUiLocked` opens with +(`src/AcDream.App/Settings/RuntimeSettingsController.cs:355`): + +```csharp +if (Gameplay.LockUI == locked && _uiLockConverged) return; +``` + +with `_uiLockConverged = true` at construction (`:185`). **Before** this commit +`locked` was computed as `!Gameplay.LockUI`, so the guard could never trip. +Now `locked` is derived from a *different* store, so whenever the persisted +`Gameplay.LockUI` already equals the newly-computed value the method +early-returns and `_runtimeTargets?.ApplyUiLock(locked)` (`:360`) never runs — +the wire bit flips, the radar (polled) follows, and `host.Root.UiLocked` does +not. Concretely: a user with `"lockUI": true` in settings.json (set by +`/lockui` in any pre-OP4 session) presses the lock toggle once after login and +nothing happens to their windows, while the server is told the UI is locked. + +--- + +## 3. SHOULD-FIX + +### S1 — `DisplayTimeStamps` only reaches a small subset of transcript lines + +`RuntimeCommunicationState.AddText` (`:219-227`) is documented as "the single +chokepoint every producer of player-visible interface text must go through", +and retail's `ClientSystem::AddTextToScroll @0x00563C50` genuinely is that +chokepoint. In acdream it is not. These producers write `ChatLog` directly and +therefore never get a timestamp: + +| Producer | Site | +|---|---| +| All heard speech (say / tell / ranged) | `Runtime/Session/LiveSessionEventRouter.cs:293` `Chat.OnLocalSpeech` | +| Emotes / soul emotes | `LiveSessionEventRouter.cs:319,321` | +| Player-kill messages | `LiveSessionEventRouter.cs:325` | +| Turbine channels (allegiance/general/trade/LFG/RP/society) | `LiveSessionEventRouter.cs:~535-556` via `RouteTurbineChat` | +| All combat text | `CombatChatTranslator` (`LiveSessionEventRouter.cs:306`) writes `social.Chat` directly | +| `@age` / `@index` / `@clist` / `@hslist` / allegiance-info replies | `Core.Net/GameEventWiring.cs:162,176,183,190,197` | +| App-side echoes | `Net/LiveSessionRuntimeFactory.cs:165,169,418` | +| Headless chat | `Headless/Hosting/HeadlessGameplayOperations.cs:242` | + +Only `ServerMessage (0xF7E0)` and the WeenieError family route through `AddText` +(`LiveSessionEventRouter.cs:310-317`, `GameEventWiring.cs:~655-668`). The gate +script's own step 13 ("send/receive a new chat line … e.g. `/who` or any inbound +tell/say. The NEW line should be prefixed") is therefore unsatisfiable for the +tell/say half. Either move the remaining producers behind `AddText` or narrow +the register/gate wording to what is actually implemented. + +Direction check (nothing leaks where it must not): the `ClientLocal`/SpewBox +branch returns before the prefix (`:213-217`) — correct and tested; there is no +chat-log-file writer in the tree (`grep` for `ChatLogFile`/`WriteChatLog` is +empty); `AddText` is display-only so nothing reaches the wire; the chat +filters in `Panels/Chat/ChatCommandRouter.cs` / `ChatInputParser.cs` match on +*outbound input*, not transcript lines, so a prefix cannot confuse them. + +### S2 — `FogMode.Off` is not honored by the sky shader + +`WeatherSystem` forces `fogMode = FogMode.Off` (`WeatherState.cs:326-327`) and +the mesh/terrain fragment shaders respect it +(`Rendering/Shaders/mesh_modern.frag:48-49` `if (mode == 0) return lit;`, +`terrain_modern.frag:160`). But `Rendering/Shaders/sky.vert:159-162` computes +`vFogFactor` from `uFogParams.x/.y` only and **never reads `uFogParams.w`**; +`sky.frag:62` then blends the dome toward the fog color. With Disable Distance +Fog ON the world is crisp to the horizon while the sky dome keeps its fog-color +band — a visible seam exactly where the option is supposed to remove one. This +is a visual-gate item (gate step 14) and needs the user's eyes either way, but +the shader asymmetry is a code fact, not a judgement call. + +### S3 — Observable-default changes are not registered + +The register hunk adds/updates `AP-194`, `TS-73`, `TS-75`–`TS-80`. None of them +records that seven options changed their *effective default source* from +`GameplaySettings.Default` (or the user's persisted settings.json) to the +retail constructor word, nor that `ViewCombatTarget`'s observable default flips +`true → false`, nor that `GameplaySettings.{AutoTarget, AutoRepeatAttack, +ViewCombatTarget, LockUI, VividTargetingIndicator, CoordinatesOnRadar, +AcceptLootPermits, ToggleRun}` are now orphaned-but-still-written. The campaign +contracted register rows for observable-default changes; these are the rows. + +### S4 — The timestamp format is culture-dependent + +`RuntimeCommunicationState.cs:225`: + +```csharp +text = DateTime.Now.ToString("H:mm:ss ") + text; +``` + +No `IFormatProvider`, so `:` is `CurrentCulture.DateTimeFormat.TimeSeparator`. +Retail's CRT `strftime("%#H:%M:%S ")` emits literal colons unconditionally. On a +machine whose culture uses `.` (e.g. `fi-FI`) acdream renders `13.05.09 `. +The new test `AddText_TimestampsTrue_PrefixesTranscriptLine` pins +`^\d{1,2}:\d{2}:\d{2} …` and would fail on such a machine — i.e. the bug is +also a latent CI/locale flake. Use `CultureInfo.InvariantCulture`. + +### S5 — The headless host's combat-option reads were not swept + +`src/AcDream.Headless/Hosting/HeadlessGameplayOperations.cs:126-127` still +hardcodes `AutoRepeatAttack => false` and `AutoTarget => true`. OP7 gave +headless bots a `characterOptions` config that writes these very bits, so a bot +configured with `AutoRepeatAttack` now sets the server bit while its own combat +loop still reads `false`. Same interface (`ICombatGameplaySettingsSource` +shape), same options, opposite direction from the graphical host after this +commit. + +### S6 — Test coverage gaps on the two changed *consumers* + +The Runtime-level OFF paths are well covered (fog unbound/false/true, timestamps +unbound/false/true + SpewBox exemption, `RunAsDefaultMovement` unbound/true/false +live-poll). What is **not** covered: + +- `DispatcherMovementInputSource.Capture()` — the file changed + (`:83`) but `tests/AcDream.App.Tests/Input/DispatcherMovementInputSourceTests.cs` + was not touched. The existing + `CapturesDispatcherHeldStateAndRetailWalkModifier` only exercises + option-unbound (⇒ `true`) + walk-held. Nothing pins the four-way truth table + (option × modifier), nor the `|| AutoRunActive` override with the option OFF. +- The per-page scoped Apply/Reset/Defaults wiring — + `OptionsPanelControllerTests.cs` was not touched, so the mechanism the commit + message calls out (`UiElement.FindDescendant` scoping) has no test that a + Chat-page Apply drives the Chat page's model rather than Character's. +- Nothing pins the login-before-seed transition (M1) — unsurprising, since the + mechanism does not exist. + +--- + +## 4. NOTE + +- **N1 — `FindDescendant` promotion changed semantics, and the scoping is + sound.** The old private `UiTabPanel.FindDescendant` skipped the root; the new + `UiElement.FindDescendant` (`src/AcDream.App/UI/UiElement.cs:312-334`) matches + the root first. `UiTabPanel:162,163,205,208` now calls the root-inclusive + version — harmless here because the panel's own id (`0x1000018D`) is not a tab + button or page id, but it is a real behavior change worth knowing. + On the OP3 re-review's duplicate-id hazard: the committed fixture + `options_panel_2100006E_1000018D.json` has 111 nodes / 38 unique ids and **16 + duplicated ids** (`0x100002CE/CF/D0` ×16 each; `0x100001FC/FD/FE` ×3). The + scoped searches are genuinely scoped — each page slot is a direct child of the + root and owns its own Apply/Reset/Defaults instance: + `/0x1000018D/0x10000211/{0x100001FC,FD,FE}`, + `/0x1000018D/0x10000213/{…}`, `/0x1000018D/0x1000050C/{…}`. A Character-page + search structurally cannot reach a Chat-page button. Separately, + `0x100001FA`/`0x100001FB` (the Character ListBox + scrollbar) are **unique**, + so `CharacterOptionsPageController`'s flat `layout.FindElement` lookups + (`:280`, `:293`) are unambiguous — but that is a property of the data, not of + the code, and nothing asserts it. +- **N2 — dead surface.** `RuntimeSettingsController.AutoTarget` / + `AutoRepeatAttack` / `ViewCombatTarget` (`:236-240`) now have zero consumers. + `GameplaySettingsState` (`Combat/LiveCombatAttackOperations.cs:23`) survives + only in `tests/AcDream.App.Tests/Combat/CombatCameraTargetSourceTests.cs:16`. +- **N3 — thread-safety is consistent; the binding fields are not volatile.** + `GetOptionBit` (`RuntimeCharacterState.cs:858-865`) reads one word through the + existing `Volatile.Read`-backed `Options1`/`Options2` properties, one static + `Dictionary` lookup, no lock, no allocation (`CharacterOptionTableEntry` is a + `readonly record struct`, `TryGetValue` uses `out`). That matches every other + read on the type. The three new `Func?` source properties are plain auto + properties written once in the `GameWindow` constructor + (`Rendering/GameWindow.cs:649-678`) before the render loop and the network + thread exist, so publication is fine in practice but is not formally + guaranteed. `WeatherSystem` is a get-only property assigned once + (`Runtime/World/RuntimeWorldEnvironmentState.cs:144,149`), so the fog binding + cannot be lost on session reset. Per-frame cost is ~3 dictionary lookups + (fog + radar ×2) + 1 in `Capture()` — negligible, and none of them is on a + Slice-I zero-allocation resolve path. +- **N4 — `FindCheckbox` is direct-children-only.** + `CharacterOptionsPageController.cs:385-392` searches the row root and its + *direct* children while the commit simultaneously promotes a recursive + `FindDescendant` for exactly this class of lookup. If a template ever nests + the checkbox one level deeper, all 50 rows silently degrade to a log line. +- **N5 — the timestamp weakens `ChatLog`'s text-keyed dedupe.** + `ChatLog.OnSystemMessage` (`src/AcDream.Core/Chat/ChatLog.cs:266-278`) dedupes + on exact text within 1 s. Timestamped and untimestamped copies of the same + logical line (one via `AddText`, one via a direct producer from S1) can no + longer match; two identical lines straddling a second boundary also stop + matching. This is an acdream-only workaround, not retail behavior, so the + consequence is a re-appearing duplicate, not a correctness break. +- **N6 — autorun ignores the new default; retail's own seam does not.** + `Run: (_movement.RunAsDefaultMovement != walking) || AutoRunActive` + (`Input/DispatcherMovementInputSource.cs:83`). The `!= walking` half is + byte-correct against retail: `CommandInterpreter::SetHoldRun @0x006b3370` + computes `(!hold_run) != (!UITogglesRun())` — i.e. `holdRun XOR ToggleRun` — + and passes that to `CMotionInterp::set_hold_run`, which is precisely where + acdream's `MovementInput.Run` lands + (`Runtime/Gameplay/PlayerMovementController.cs:1166-1168`). The wire encoding + is untouched and stays retail-correct in both option states: `Run` → + `IsRunning` → `HoldKey.Run`/`HoldKey.None` + `ForwardSpeed` + (`Runtime/Gameplay/LocalPlayerOutboundController.cs:234-248`), so no + double-inversion. There is also no collision with the `ToggleRunLock` + keybind latch — that drives `_autoRunActive`, a separate field, and the + property's doc comment says so explicitly. What is newly *reachable* is the + `|| AutoRunActive` clause: `CommandInterpreter::SetAutoRun @0x006b4850` sets + `auto_run` and calls `ApplyCurrentMovement()` — it does **not** force run — + so with the option OFF an autorunning acdream player runs where retail would + walk. That `||` predates this commit (2026-07-30 movement-parity audit) but + was unobservable while the default was hardcoded. +- **N7 — citation accuracy.** `RuntimeLocalPlayerMovementState`'s doc comment + names "retail `PlayerOption RunAsDefaultMovement`". No such member exists — + `acclient.h:4165` spells id `0xA` `ToggleRun_PlayerOption`, and + `RunAsDefaultMovement` is ACE's name for the bit. The binding + (`GameWindow.cs:677`) and the row's `RetailName` (`"ToggleRun"`, which is what + hashes `ID_PlayerOption_ToggleRun` — confirmed at pseudo-C `:171586`) are both + correct; only the prose is wrong. +- **N8 — `Defaults` can emit a burst of 21 immediate `0x0005` sends.** + `OptionPage.Defaults()` calls `RestoreDefaultValue()` on all 50 rows, each of + which publishes `SetSingleCharacterOptionRuntimeCmd`. `TrySetOption` no-ops on + unchanged values, so the worst case is bounded by the auto-save set (21 ids). + Retail does the same thing; recorded here only so a future wire-rate reviewer + does not treat it as a bug.