diff --git a/docs/research/2026-08-10-op1-review-blast.md b/docs/research/2026-08-10-op1-review-blast.md new file mode 100644 index 00000000..b4c7651d --- /dev/null +++ b/docs/research/2026-08-10-op1-review-blast.md @@ -0,0 +1,477 @@ +# Campaign OP slice OP1 — regression / blast-radius review + +**Commit under review:** `86c0a7e0` "feat(runtime,net): Campaign OP slice OP1 — +full character-option table, dirty model, real 0x01A1 blob builder" +**Reviewer lens:** what this change can break OUTSIDE its stated scope. +Retail-mechanism fidelity (mask transcription, auto-save split, `PlayerModule::Pack` +field order) is a sibling reviewer's lane and is only touched here where a +mismatch would be a *regression* in already-working behavior. +**Date:** 2026-08-10 · **Mode:** read-only (no build, no test run, no launch) + +**Verdict: APPROVE-WITH-FIXES.** + +--- + +## 0. Consumer enumeration (coverage record for the re-reviewer) + +Both axes walked, per `feedback_blast_radius_single_lens.md`: (a) every caller of +each changed surface, and (b) every *other* caller of each surface the change +routes through. + +### 0.1 `RuntimeCharacterOptionsState` — every consumer + +| Consumer | Site | Member used | Changed by OP1? | +|---|---|---|---| +| `TurbineChatMembershipGate.Evaluate` | `src/AcDream.Runtime/Gameplay/TurbineChatMembershipGate.cs:108-129` | `Options1`, `Options2` | No — masks bit-identical (§1.1) | +| `LiveSessionCommandRouter.RouteTurbineChat` | `src/AcDream.App/Net/LiveSessionCommandRouter.cs:346` | passes `CharacterState.Options` into the gate | No | +| `LiveSessionEventRouter` (PlayerDescription) | `src/AcDream.Runtime/Session/LiveSessionEventRouter.cs:212` | `Replace(options1, options2)` | **Not updated** — see S5 | +| `RuntimeCharacterState.CaptureOwnership` | `src/AcDream.Runtime/Gameplay/RuntimeCharacterState.cs:198, 211-213` | `Snapshot`, `DefaultOptions1/2` | **Ledger not widened** — see S3 | +| `RuntimeCharacterState.View` (`IRuntimeCharacterView`) | `RuntimeCharacterState.cs:512` | `Snapshot` | No | +| `GameRuntimeEvents` canonical checkpoint string | `src/AcDream.Runtime/GameRuntimeEvents.cs:177` | `Options.Revision` | Revision now *stops* advancing on a redundant toggle, and *starts* advancing for 47 previously-unmodeled ids — see N1 | +| `InteractionRetainedUiComposition` | `src/AcDream.App/Composition/InteractionRetainedUiComposition.cs:318` | `Character.Options.DragItemOnPlayerOpensSecureTrade` | No | +| `RuntimeCharacterState.ResetSession` / `Dispose` | `RuntimeCharacterState.cs:431, 457` | `Options.ResetSession()` | Now also clears `_isDirty` — correct (§3) | +| `RuntimeGenerationReset` (J8 reset transaction) | `src/AcDream.Runtime/RuntimeGenerationReset.cs:272` | `_character.ResetSession` | Reaches the new dirty clear — correct (§3) | +| `GameRuntime` construction | `src/AcDream.Runtime/GameRuntime.cs:199-200` | now passes `dependencies.TimeProvider` | New; no other construction site | +| `LiveSessionRuntimeFactory.SendSingleCharacterOption` | `src/AcDream.App/Net/LiveSessionRuntimeFactory.cs:347-352` | `SetOptionBit` → **`TrySetOption`** | Rewritten | +| `LiveSessionRuntimeFactory.SaveCharacterOptionsIfDirty` | `LiveSessionRuntimeFactory.cs:357-370` | `TryFlush` | New | +| `DirectGameRuntimeCommandAdapter.SetSingleOption` | `src/AcDream.Runtime/Session/DirectGameRuntimeCommandAdapter.cs:666-670` | **`TrySetOption`** (was: wire send only) | Rewritten | +| `DirectGameRuntimeCommandAdapter.SaveOptions` | `DirectGameRuntimeCommandAdapter.cs:684-697` | `TryFlush` | New | +| `CharacterOptionsBlobSource.Capture` | `src/AcDream.Runtime/Gameplay/CharacterOptionsBlobSource.cs:41-47` | `Options1`, `Options2` | New | + +No production caller of `SetOptionBit` survives outside `TrySetOption` +(verified by grep). The only remaining direct callers are tests +(`TurbineChatMembershipGateTests.cs:228/247`, +`RuntimeCharacterStateTests.cs:249-282`, +`LiveSessionCommandRouterTests.cs:497`). + +### 0.2 `CharacterOptionId` (6 → 53 members) — every consumer + +| Consumer | Site | Effect of the widening | +|---|---|---| +| `RuntimeSettingsController.SaveChat` | `src/AcDream.App/Settings/RuntimeSettingsController.cs:540-552` | none — still names the 5 Options2 chat ids by member | +| `RetailClientCommandCatalog.JoinLeaveTags` | `src/AcDream.UI.Abstractions/Panels/Chat/RetailClientCommandCatalog.cs:302-312` | none — hard-coded raw `0x1B/0x23/0x24/0x25/0x26/0x2E` uints with comments; values re-verified against the widened enum, all six match | +| `CharacterOptionTable` | `src/AcDream.Runtime/Gameplay/CharacterOptionTable.cs` | new — keys the table | +| Tests (`RuntimeCharacterStateTests`, `RuntimeSettingsControllerTests`, `LiveSessionCommandRouterTests`, `DirectGameRuntimeCommandAdapterTests`, `TurbineChatMembershipGateTests`) | various | no exhaustiveness assumption anywhere | + +No `switch` over `CharacterOptionId`, no `Enum.GetValues()` +in production, no name-based serialization. The widening is safe — the old +6-case `switch` in `SetOptionBit` was the only exhaustiveness-shaped consumer +and it is gone. + +### 0.3 `PlayerDescriptionParser.CharacterOptions1/2` — every consumer + +| Consumer | Site | Touched? | +|---|---|---| +| `TurbineChatMembershipGate` | `TurbineChatMembershipGate.cs:109-129` | no | +| `RuntimeSettingsController.SyncChatFromServerOptions` | `RuntimeSettingsController.cs:591-599` | no | +| `RuntimeCharacterOptionsState.DefaultOptions1` | `RuntimeCharacterState.cs:635` | no | +| `RuntimeCharacterOptionsSnapshot.DragItemOnPlayerOpensSecureTrade` | `RuntimeCharacterState.cs:618-621` | no | +| `ChatSettings` doc | `src/AcDream.UI.Abstractions/Panels/Settings/ChatSettings.cs:20, 91` | no | + +This enum is now a **second, independent** definition of the same bits that +`CharacterOptionTable` defines — see S6. + +### 0.4 `IRuntimeCharacterCommands` implementers (interface gained `SaveOptions`) + +- `src/AcDream.Runtime/Session/DirectGameRuntimeCommandAdapter.cs:29` — updated +- `src/AcDream.App/Runtime/CurrentGameRuntimeCommandAdapter.cs:28` — updated +- `tests/AcDream.App.Tests/Composition/InteractionUiRuntimeSourcesTests.cs:219` — fake, updated + +No plugin-facing implementer (`AcDream.Plugin.Abstractions` is BCL-only and does +not see `AcDream.Runtime`). `AcDream.Headless` has no character-option command +surface at all (grep over `src/AcDream.Headless` finds only config-file +`session.Character.*`), so bots reach these verbs through +`GameRuntime.Commands.Character` directly. + +### 0.5 Routes that previously reached the deleted `SendSingleCharacterOption` closure + +| Route | Chain | Post-OP1 behavior | +|---|---|---| +| Settings Chat toggles | `RuntimeSettingsController.PublishHearOptionChange` (`:561-566`) → `RuntimeSettingsTargets.SetSingleCharacterOption` (`:321-322`) → `SetSingleCharacterOptionRuntimeCmd` → `LiveSessionCommandRouter` (`:173-177`) → `bindings.SendSingleCharacterOption` → `TrySetOption` | one local write, then one `0x0005` — **unchanged** for all five ids (all `IsAutoSave`), except the new unchanged-value no-op (N1) | +| `@join` / `@leave` | `ClientCommandController` (`:285-292`) → `Bindings.SetSingleCharacterOption` (wired at `LiveSessionRuntimeFactory.cs:491`) → same closure → `TrySetOption` | same — all six join/leave ids are `IsAutoSave = true` | +| Router's guarded client-command projection | `LiveSessionCommandRouter.BuildGuardedClientCommands` (`:485-486`) → `InvokeClient` → same closure | same | +| Graphical Runtime command | `CurrentGameRuntimeCommandAdapter.SetSingleOption` (`:668-695`) → `_commands.Publish` → router → same closure | same, plus a new pre-publish unknown-id rejection | +| Headless / bot | `DirectGameRuntimeCommandAdapter.SetSingleOption` (`:652-675`) | **fixed** — previously sent the wire message with no local write | + +`LiveCommandBus.Publish` (`src/AcDream.UI.Abstractions/LiveCommandBus.cs:48-63`) +is **synchronous** dispatch, so there is no queue and no reordering window +between a `SetSingleCharacterOptionRuntimeCmd` and a +`SaveCharacterOptionsRuntimeCmd`; the local bit write completes inside the +adapter call. **No double-write and no dropped write on any route** — each route +performs exactly one `TrySetOption` and `TrySetOption` performs exactly one +`SetOptionBit`. + +--- + +## 1. Prompt item 1 — is the CH3/CH4 behavior bit-identical? + +### 1.1 Mask equivalence (verified) + +The six ids the old 6-case `switch` modeled resolve to **exactly** the same +(word, mask) pairs in `CharacterOptionTable`: + +| id | old switch (`RuntimeCharacterState.cs`, pre-OP1) | `CharacterOptionTable.cs` | match | +|---|---|---|---| +| `0x1B` ListenToAllegianceChat | Options1 `CharacterOptions1.HearAllegianceChat = 0x40000000` (`PlayerDescriptionParser.cs:215`) | `:138` Options1 `0x40000000` | yes | +| `0x23` ListenToGeneralChat | Options2 `0x00000100` (`:232`) | `:146` Options2 `0x00000100` | yes | +| `0x24` ListenToTradeChat | Options2 `0x00000200` (`:233`) | `:147` `0x00000200` | yes | +| `0x25` ListenToLFGChat | Options2 `0x00000400` (`:234`) | `:148` `0x00000400` | yes | +| `0x26` ListenToRoleplayChat | Options2 `0x00000800` (`:235`) | `:149` `0x00000800` | yes | +| `0x2E` ListenToSocietyChat | Options2 `0x00080000` (`:236`) | `:157` `0x00080000` | yes | + +All six carry `IsAutoSave = true`, so all six still take the immediate-`0x0005` +branch of `TrySetOption` (`RuntimeCharacterState.cs:724-725`). The +`TurbineChatMembershipGate` reads the same bits it always did. **CH3's gate and +CH4's `@join`/`@leave` are behaviorally identical on every changed-value +toggle.** + +I also hand-verified all 53 `(IsOptions1, Mask)` pairs are pairwise distinct, and +that OR-ing the `ClientDefault = true` rows reproduces `0x50C4A54A` / +`0x00008700` (the table's own test at `CharacterOptionTableTests.cs:120-140` +asserts this, and it holds independently). + +### 1.2 The one real behavior change (N1) + +`TrySetOption` early-returns when the bit already holds the requested value +(`RuntimeCharacterState.cs:718-720`). The pre-OP1 closure wrote and sent +unconditionally. So: + +- `/join general` when already listening now sends **nothing** (was: a redundant + `0x0005`). +- A Settings save whose persisted `ChatSettings` diff says "changed" but whose + Runtime bit already matches now sends **nothing**. + +I verified against retail: `PlayerModule::SetHearGeneralChat @0x005D35C0` +(`docs/research/named-retail/acclient_2013_pseudo_c.txt:486939`) opens with +`if ((((int8_t)(this->options2_ >> 8)) & 1) == arg2) return;` — the early return +is real, and `ClientCommunicationSystem::DoJoinChat @0x0056F510` emits no +confirmation text of its own, so retail's repeat `/join` is also silent. **This +change is retail-faithful.** It is listed as a NOTE, not a defect, because it +removes acdream's only *manual* resync path if the local bit and ACE's stored +option ever diverge (e.g. a `0x0005` that ACE dropped pre-`LoginComplete`) — +retail has the identical limitation, so this is a fidelity-consistent +regression in robustness only. + +--- + +## 2. Prompt item 2 — the deleted closure and the two adapters + +Traced above (§0.5). Summary: **exactly one local write and at most one send on +every route; no double-apply, no dropped write, no reordering.** + +On the specific "DIRECT adapter double-apply when a PlayerDescription re-seed +races a toggle": `TrySetOption` reads the word (`:718`), compares, then calls +`SetOptionBit` which **re-reads** the word fresh (`:754/:759`) before the +read-modify-write. A `Replace` landing between the two reads therefore cannot +produce a double-apply — the OR/AND-NOT is applied once, to whichever word is +current. The opposite interleaving (`SetOptionBit` writes, then `Replace` +overwrites) *loses* the local write; that is a pre-existing shape inherited from +CH4 for auto-save ids, but OP1 extends it to batched ids where it now has a new +consequence — see S5. + +--- + +## 3. Prompt item 3 — dirty model, reset, cross-character contamination + +**Clean.** `RuntimeCharacterOptionsState.ResetSession` clears `_isDirty` +(`RuntimeCharacterState.cs:826-827`), and it is reached from both owners of +character lifetime: + +- generation replacement / reconnect: `RuntimeGenerationReset.cs:272` + `Advance(state, _character.ResetSession)` → `RuntimeCharacterState.ResetSession` + (`:425-443`) → `Try(Options.ResetSession, ...)` (`:431`) +- terminal disposal: `RuntimeCharacterState.Dispose` (`:457`) + +So a dirty flag from character A cannot survive into character B's generation, +and cannot cause a `SaveOptions` blob to be sent with B's identity carrying A's +intent. `_firstDirtiedAt` is deliberately left stale but is unreachable while +`_isDirty == false` (`FirstDirtiedAt` getter, `:676-679`) and is overwritten by +the next `MarkDirty`. Pinned by +`RuntimeCharacterStateTests.ResetSession_ClearsDirtyState` (`:427-439`). + +Two gaps around this, both S-grade: S3 (the ownership ledger does not observe +the new field) and S5 (`Replace` does not). + +--- + +## 4. Prompt item 4 — is the blob echo stale? + +**No. The echo reads the canonical live owners, not a parser-side copy.** +Verified source by source: + +| Echo field | Read from | Canonical? | +|---|---|---| +| `Options1/Options2` | `character.Options` (`CharacterOptionsBlobSource.cs:42-43`) | yes — the same object `TrySetOption` writes and the gate reads | +| `Shortcuts` | `shortcuts.Items` — `RuntimeInventoryState.Shortcuts` (`RuntimeInventoryState.cs:81`), the J4.5 `ShortcutStore` | yes — `CurrentGameRuntimeCommandAdapter.AddShortcut` (`:426-428`) goes through `RuntimeInventoryState.TryAddShortcut` (`:119-140`) which writes `Shortcuts.Set(entry)`; `RemoveShortcut` likewise (`:161`) | +| `FavoriteSpells` | `character.Spellbook.GetFavorites(tab)` × 8 (`CharacterOptionsBlobSource.cs:38-39`) | yes — `CurrentGameRuntimeCommandAdapter.AddFavorite/RemoveFavorite` (`:477, :508`) mutate the same `Spellbook` | +| `DesiredComponents` | `character.Spellbook.DesiredComponents` (`Spellbook.cs:219`) | yes — same J4.3 spellbook the UI mutates | +| `SpellbookFilters` | `character.Spellbook.SpellbookFilters` (`Spellbook.cs:221`) | yes — `SpellbookWindowController.cs:254` writes via `SetSpellbookFilters` | + +`ShortcutStore.Items` returns a `Volatile.Read` of an immutable array +(`ShortcutStore.cs:37`) and `GetFavorites` returns `.ToArray()` +(`Spellbook.cs:216`), so those two are snapshot-safe. `DesiredComponents` +returns the **live** dictionary — see N4. + +**There is, however, an unrelated wipe hazard on the same verb: see M1.** + +--- + +## 5. Prompt item 5 — wire safety + +Both adapters gate on generation + `IsInWorld`: +`CurrentGameRuntimeCommandAdapter.Validate(expectedGeneration, requireWorld: true)` +(`:791-820`) and `DirectGameRuntimeCommandAdapter.Validate` (`:1111-1132`, which +additionally requires a non-null route and session). The graphical route then +passes `LiveSessionCommandRouter.SendIfActive` (`:523-532`), which is `false` +before `Activate()` and after `Dispose()`. **Post-teardown sends are impossible +on both hosts.** + +Pre-`LoginComplete` is a different story. `LiveSessionController` sets +`_inWorld = true` at `:587`, immediately after `_operations.EnterWorld(...)` +returns at `:580` — i.e. as soon as the `CharacterEnterWorld` body is *sent*. +`GameActionLoginComplete` is only emitted later, from the inbound stream, when +the local player's own `CreateObject` is accepted +(`RuntimeLiveEntitySessionController.cs:176-183` for the content-less host, +`:858` for the graphical placement conductor). So `IsInWorld == true` does **not** +imply `LoginComplete` has been sent, and there is a real window in which a +`0x01A1` would be silently dropped by ACE (lane C §5). This is the same window +`0x0005` has had since CH3, so OP1 does not newly create it — but it does newly +route a *destructive* message through it. See **M1**. + +There is currently **no production caller of `SaveOptions`** — grep finds only +the two adapters, the router registration, and tests. `MarkDirty` is likewise +unreachable in production today: the only production entrances +(`@join`/`@leave` via `JoinLeaveTags`, and the five Settings Chat toggles) all +carry auto-save ids. So today's *live* blast radius of the blob path is +effectively zero; M1/S2 are pre-emptive against the very next slice. + +--- + +## 6. Findings + +### MUST-FIX + +**M1 — `SaveOptions` has no "server state seeded yet" gate; firing it before the +first `PlayerDescription` overwrites the character's server-side options with +client defaults.** +`RuntimeCharacterOptionsState` starts at the *client* constructor defaults +`DefaultOptions1 = 0x50C4A54A` / `DefaultOptions2 = 0x00948700` +(`src/AcDream.Runtime/Gameplay/RuntimeCharacterState.cs:634-636`) and is only +corrected to server truth when `Replace` runs from +`LiveSessionEventRouter.cs:212`. The command gate is `IsInWorld` +(`DirectGameRuntimeCommandAdapter.cs:1128`, +`CurrentGameRuntimeCommandAdapter.cs:800`), which as shown in §5 goes true +before the description arrives. A bot (or, after TS-71, a logout hook) that does +"toggle a batched option, then `SaveOptions`" inside that window sends a +`0x01A1` whose `options1`/`options2` — the two fields ACE actually *stores* — +are the client defaults, not the character's. `spellbookFilters` is also always +packed (header bit `0x020` is unconditional, +`src/AcDream.Core.Net/Messages/SocialActions.cs:74-77`) and would be `0`. +That is exactly the "wipe server-side state" failure class this review was asked +to treat as MUST-FIX; it is simply reached through the *options* fields rather +than the echo fields. Reachability is narrow today (no production caller, and +`MarkDirty` is unreachable from any wired entrance), which is why the verdict is +APPROVE-WITH-FIXES rather than REJECT — but the guard must land before TS-71 +wires the logout/timer triggers or before any Options-panel Apply button exists. +Suggested fix: a `HasServerSeed` latch set by `Replace` and required by +`TryFlush`/`TryFlushIfAutoSaveDue` (cleared by `ResetSession`), with a test. + +### SHOULD-FIX + +**S1 — the CH3/CH4 regression test no longer exercises the production closure; it +now mirrors the *pre-OP1* shape.** +`tests/AcDream.App.Tests/Net/LiveSessionCommandRouterTests.cs:493-499` substitutes +its own binding — `characterState.Options.SetOptionBit(id, value); sent.Add(...)` +— and its comment still says "Mirrors LiveSessionRuntimeFactory. +CreateCommandBindings' shared SendSingleCharacterOption local function". After +OP1 that function is `TrySetOption` with an unchanged-value early return +(`LiveSessionRuntimeFactory.cs:347-352`), so the mirror has silently drifted +from the thing it is supposed to protect. No test anywhere drives the real +closure. This is precisely the "tests that pin a wrong shape and look green" +failure the commit message cites as the reason the CH3 `0x01A1` builder died. +Fix: make the test construct the real binding, or extract the closure so it can +be exercised directly. + +**S2 — `TryFlush` / `TryFlushIfAutoSaveDue` invoke a caller-supplied callback +while holding `_dirtyGate`, which sets up a lock inversion that TS-71's own +follow-up will trigger.** +`RuntimeCharacterState.cs:766-777` and `:786-796` call `flush()` inside +`lock (_dirtyGate)`. Today every flush originates *inside* +`LiveSessionCommandRouter.SendIfActive`, which already holds `_gate` +(`LiveSessionCommandRouter.cs:523-532`), so the order is consistently +`_gate → _dirtyGate`. The natural TS-71 wiring — a per-frame +`Options.TryFlushIfAutoSaveDue(() => _commands.Publish(new SaveCharacterOptionsRuntimeCmd()))` +on the update thread — inverts it: that thread would hold `_dirtyGate` and then +want `_gate`, while the render thread holds `_gate` (routing a +`SetSingleCharacterOptionRuntimeCmd`) and wants `_dirtyGate` via `MarkDirty`. +Deadlock. It also means a UDP send happens under the lock, blocking the +`IsDirty`/`FirstDirtiedAt` getters. Fix: decide and clear under the lock, invoke +the callback outside it (and document what happens if the send throws — today's +`flush(); _isDirty = false;` ordering correctly *keeps* the module dirty on a +throw, which is the behavior to preserve). + +**S3 — the ownership ledger does not observe the new dirty state.** +`RuntimeCharacterOwnershipSnapshot.IsConverged` +(`RuntimeCharacterState.cs:26-40`) checks `OptionsAreDefaults` but has no +`OptionsAreClean`. The project's J3.6/J8 discipline is "the combined ownership +ledger converges to zero"; a new piece of session state that reset must clear is +exactly what that ledger exists to catch. `CaptureOwnership` (`:167-225`) should +carry `IsDirty == false`. + +**S4 — `IRuntimeCharacterCommands.SaveOptions` has host-dependent result +semantics, and encodes a boolean in a typed object-id field.** +`DirectGameRuntimeCommandAdapter.SaveOptions` returns +`primaryObjectId: flushed ? 1u : 0u` (`:701`); `CurrentGameRuntimeCommandAdapter. +SaveOptions` always returns `0` (`:704-707`) because its flush is deferred onto +the router. A consumer of the shared interface therefore gets different answers +from the two hosts. Worse, `PrimaryObjectId` flows straight into the K2 bot-facing +ordered event stream (`GameRuntimeEventHub.cs:151-161`, +`GameRuntimeEvents.cs:36/144/242`), where `1` reads as object guid `0x00000001`. +Either add a dedicated result field or drop the encoding and make both hosts +return `0`. + +**S5 — a PlayerDescription re-seed discards a pending batched change but leaves +the module dirty.** +`RuntimeCharacterOptionsState.Replace` (`:684-689`) overwrites both words +wholesale and does not touch `_isDirty`. If the user flips a batched option and a +fresh `PlayerDescription` lands before the flush, the local intent is lost while +`IsDirty` stays `true`; the eventual `SaveOptions` then echoes the server's own +words back at it. Harmless on the wire, silently wrong for the user. No test +covers `Replace` against a dirty module. At minimum this needs a decision +recorded (retail's `PlayerModule` is likewise clobbered by a re-seed, so +"reproduce it" may be the right answer) plus a test that pins whichever +behavior is chosen. + +**S6 — the same bits are now defined twice, with nothing asserting they agree.** +`PlayerDescriptionParser.CharacterOptions1/2` +(`src/AcDream.Core.Net/Messages/PlayerDescriptionParser.cs:206-237`) is the +*read* path — `TurbineChatMembershipGate.cs:109-129` and +`RuntimeSettingsController.cs:591-599` both key off it. `CharacterOptionTable` is +now the *write* path. I verified by hand that all six overlapping bits agree +today, but the two definitions are in different projects with no cross-check. +Edit one without the other and the write path and the membership gate diverge +silently — the exact CH3 failure mode. Add a conformance test asserting +`CharacterOptionTable`'s six chat rows equal the `PlayerDescriptionParser` enum +values (or have one derive from the other). + +### NOTE + +**N1 — observable behavior deltas from the unchanged-value early return.** +Repeat `/join`/`/leave` and a redundant Settings save now emit no `0x0005` +(retail-faithful, verified against `SetHearGeneralChat @0x005D35C0` and +`DoJoinChat @0x0056F510`). Two second-order effects worth knowing: the manual +resync path for a client/server option desync is gone, and +`Options.Revision` — which is embedded in the canonical checkpoint string at +`GameRuntimeEvents.cs:177` used by the connected lifecycle/reconnect gates — no +longer advances on a redundant toggle, while it *now can* advance for 47 +previously-unmodeled ids. Any recorded checkpoint baseline that captured a +redundant toggle would drift. + +**N2 — cross-project doc-comment `cref`s in `SocialActions.cs`.** +`src/AcDream.Core.Net/Messages/SocialActions.cs:348` writes +`` from +`AcDream.Core.Net`, which does not (and must not) reference `AcDream.Runtime`. +`:201` uses `` for a parameter (should be +`paramref`). Both are inert today because no project sets +`GenerateDocumentationFile`, but every project sets +`TreatWarningsAsErrors` — enabling XML docs later would turn these into build +breaks. The first is also an upward layering reference in documentation. + +**N3 — `CharacterOptionTable`'s static state is outside the contract guard.** +`GameRuntimeContractTests.J4GameplayOwnersHaveNoStaticMutableSessionState` +(`tests/AcDream.Runtime.Tests/GameRuntimeContractTests.cs:218-246`) enumerates an +explicit type list that does not include `CharacterOptionTable`, whose +`private static readonly Dictionary` (`:83`) and `All` property (`:93-94`) are +process-wide shared state. Immutable in practice, and the commit correctly made +`AutoSaveDelay` a property to satisfy that guard on +`RuntimeCharacterOptionsState` — but the table itself is unguarded. Also, `All`'s +initializer reads `Entries`, so the two declarations are order-coupled (`:83` +before `:93`); correct today, fragile under reordering. + +**N4 — `BuildSetCharacterOptions` sizes and then enumerates a live dictionary.** +`SocialActions.cs:232` and `:255-256` read `desiredComponents.Count` for the +buffer size and then `foreach` the same collection, and +`CharacterOptionsBlobSource.Capture` hands it +`Spellbook.DesiredComponents`, which returns the **live** `_desiredComponents` +reference (`src/AcDream.Core/Spells/Spellbook.cs:219`), not a copy. A mutation +between sizing and writing throws (span slice) or mis-sizes the body. Same-thread +under the #368 single-update-thread contract, so not reachable today; copying in +`Capture` would close it for a couple of allocations. + +**N5 — `LockUI` now has two homes.** +`CharacterOptionId.LockUI` (`0x33`) is modeled as an auto-save option in the +table (`CharacterOptionTable.cs:162`), while the Settings "Lock UI" toggle still +routes to the purely local `RuntimeSettingsTargets.ApplyUiLock` +(`src/AcDream.App/Settings/RuntimeSettingsTargets.cs:305`, reached from +`RuntimeSettingsController.cs:349/509`). Nothing wrong today; it becomes a +two-sources-of-truth bug the moment the Options panel binds `0x33`. + +**N6 — the echo's canonicality is not pinned by a test.** +`DirectGameRuntimeCommandAdapterTests.SaveOptions_FlushesTheDirtyBlobThenNoOpsWhenClean` +(`tests/AcDream.Runtime.Tests/Session/DirectGameRuntimeCommandAdapterTests.cs:347-375`) +asserts only the opcode at offset 8. Nothing asserts that a shortcut added +in-session appears in the flushed blob. A regression from +`CharacterOptionsBlobSource.Capture`'s canonical reads to a parser-side copy — +the failure the prompt calls out — would pass every current test. + +**N7 — no uniqueness test on the table.** +`CharacterOptionTableTests` pins count, contiguity, the auto-save split, the +client-default split, and the reconstructed default words, but not "no two +entries share the same `(IsOptions1, Mask)`". The reconstruction test cannot +catch a duplicate because `|=` is idempotent. I verified uniqueness by hand +across all 53 rows; a one-line test would make it permanent. + +**N8 — the golden-vector test's round-trip couples an outbound builder to an +inbound parser.** +`SocialActionsTests.BuildSetCharacterOptions_RoundTripsThroughPlayerDescriptionParser` +(`tests/AcDream.Core.Net.Tests/Messages/SocialActionsTests.cs:242-296`) validates +the body against acdream's own `PlayerDescriptionParser`. Where the parser is +lenient — notably the `PackableHashTable` size word, which +`SocialActions.cs:251-253` deliberately writes as a bare count while retail +writes `(tableSize << 16) | count` — a lenient parser makes the round trip green +regardless. The hand-computed golden vector (`:170-198`) is the load-bearing +test; the round trip is corroboration only. (Whether the bare count is the right +wire value is the fidelity reviewer's call.) + +--- + +## 7. What I checked and found clean + +- No production caller of `SetOptionBit` outside `TrySetOption`. +- No `switch`/`Enum.GetValues` exhaustiveness dependency on `CharacterOptionId`. +- `RetailClientCommandCatalog.JoinLeaveTags`' six hard-coded ids all still map to + auto-save rows, so `@join`/`@leave` still sends `0x0005`. +- `LiveCommandBus.Publish` is synchronous — no reorder window between a toggle + and a flush. +- `SaveCharacterOptionsRuntimeCmd` is generation/activity gated identically to + every other router command (`SendIfActive`), pinned by + `LiveSessionCommandRouterTests.cs:444-456`. +- `Character` domain operation ids: `Advance` uses `(int)RuntimeAdvancementKind` + = 0–3 (`GameRuntimeCommands.cs:222-228`), options use 4 and 5 — no collision on + either adapter. +- Both adapters were updated for the new interface member; the only other + implementer is a test fake. +- The J8 generation-reset transaction reaches the new dirty clear. +- `TryFlush` leaves the module dirty if the send throws (correct direction). +- `SaveCharacterOptionsIfDirty` captures the per-session `WorldSession` passed + into `CreateCommandBindings`, so no stale-session capture across generations. +- All 53 table masks are pairwise distinct; `ClientDefault` reconstruction + reproduces `0x50C4A54A` / `0x00008700`; the constructor default `0x00948700` + decomposes exactly as the register row claims. +- Register rows for TS-71, AP-193 and AP-194 are present in the same commit. + +--- + +## 8. Verdict + +**APPROVE-WITH-FIXES.** Nothing in the diff regresses behavior that works +today: the six chat masks are bit-identical, every route still performs exactly +one local write followed by at most one send, the J8 reset clears the new dirty +state, and the blob echo reads the canonical live owners rather than a +parser-side copy. The headless local-write gap is a genuine fix. The blocking +item (M1) is a missing readiness gate on a newly-introduced *destructive* wire +verb that is not yet production-called — it must land before TS-71 wires the +logout/timer triggers. S1 (a regression test that now mirrors the old shape), +S2 (a lock-inversion landmine aimed squarely at TS-71), and S6 (duplicated bit +definitions across projects with no cross-check) are the other items I would +want closed inside this campaign rather than deferred. diff --git a/docs/research/2026-08-10-op1-review-mechanism.md b/docs/research/2026-08-10-op1-review-mechanism.md new file mode 100644 index 00000000..cf1ba0f7 --- /dev/null +++ b/docs/research/2026-08-10-op1-review-mechanism.md @@ -0,0 +1,447 @@ +# Campaign OP slice OP1 — mechanism-faithfulness review + +**Commit under review:** `86c0a7e0` ("feat(runtime,net): Campaign OP slice +OP1 — full character-option table, dirty model, real 0x01A1 blob builder"). +**Lens:** retail-mechanism faithfulness and contract fidelity ONLY. Blast +radius is a sibling reviewer's lane. +**Mode:** read-only. No build, no test run, no launch. Git reads only. +**Date:** 2026-08-10. + +**Verdict: APPROVE-WITH-FIXES.** 1 MUST-FIX, 2 SHOULD-FIX, 7 NOTE. + +The ported data is correct. I verified **all 53 table rows** (not a sample) +against the verbatim retail header, re-decoded both byte tables from the +research myself, and recomputed the golden wire vector byte-for-byte — +every one matches. The defects are (a) a register row whose stated +*reason* for a deferral is factually wrong, and (b) an omitted half of the +retail function the code claims to mirror "exactly". + +--- + +## 1. Row-by-row table check — 53 / 53 rows verified + +`src/AcDream.Runtime/Gameplay/CharacterOptionTable.cs:111-165`. + +**Method.** I did not trust the research doc's summary tables. For each of +the 53 rows I checked four independent things: + +1. **id → name** against `docs/research/named-retail/acclient.h:4162-4218` + (`enum PlayerOption`, read directly). +2. **name → word + mask** against `acclient.h:3404-3436` + (`enum CharacterOption` = the Options1 bitfield) and + `acclient.h:3451-3481` (`enum CharacterOptions2`), read directly. +3. **IsAutoSave** against my own decode of the 0x34-byte jump table quoted + at `docs/research/2026-08-10-set-character-options-wire.md` §3.2 — I + indexed the 52 bytes myself rather than reading off the doc's id list. +4. **ClientDefault** against my own decode of the 0x2B-byte table quoted at + the same doc's §8.2 / `2026-08-10-character-options-map.md` §1.4. + +**Result: 53/53 rows correct. Zero altered ids, zero altered masks, zero +altered flags.** Specifically: + +| Column | Expected | Found in code | Verdict | +|---|---|---|---| +| id coverage | contiguous `0x00..0x34` | contiguous `0x00..0x34`, 53 entries | ✅ | +| word + mask | 53 rows from `acclient.h` | 53 rows | ✅ all 53 | +| `IsAutoSave` | 21 ids: `00 01 02 0F 10 11 12 19 1B 23 24 25 26 27 2A 2B 2E 2F 31 32 33` | identical set, count 21 | ✅ | +| `ClientDefault` | 16 ids: `00 02 06 08 0A 0D 0E 0F 14 15 19 1B 23 24 25 2A` | identical set, count 16 | ✅ | + +My own decode of the §3.2 table (`00`⇒auto-save) yields exactly the 21 ids +above; my own decode of the §8.2 table (43 bytes, indices `0x00..0x2A`, +`00`⇒true) yields exactly the 16 above, with `0x2B..0x34` falling off the +end to `false`. Both match the code. + +**Independent arithmetic cross-check.** OR-ing every `ClientDefault=true` +row's mask into its own word by hand: + +- Options1 = `0x02|0x08|0x40|0x100|0x400|0x2000|0x8000|0x40000|0x400000|0x800000|0x10000000|0x40000000` + = **`0x50C4A54A`** ✅ (matches the byte-verified constructor literal at + `PlayerModule::PlayerModule @0x005D51F0`) +- Options2 = `0x100|0x200|0x400|0x8000` = **`0x00008700`** ✅ + +This is a genuine cross-confirmation of the id→mask assignment for those 16 +rows, exactly as the type's XML doc claims +(`CharacterOptionTable.cs:41-45`). + +**Deeper verification of the five highest-risk rows.** The rows where a +transposition would be invisible are the ones whose mask is *out of order* +relative to the id. I read their retail setter bodies directly out of +`docs/research/named-retail/acclient_2013_pseudo_c.txt` rather than relying +on the header: + +| Retail setter | Writes | Tail-jumps with id | Code row | Verdict | +|---|---|---|---|---| +| `PlayerModule::SetIgnoreTradeRequests @0x005D2B90` | `options_ \|= 0x20000` | `arg2 = 3` | `IgnoreTradeRequests, O1, 0x00020000` | ✅ | +| `PlayerModule::SetDisplayAge @0x005D37E0` | `options2_ \|= 0x20` | `arg2 = 0x1d` | `DisplayAge, O2, 0x00000020` | ✅ | +| `PlayerModule::SetDisplayNumberDeaths @0x005D38D0` | `options2_ \|= 0x10` | `arg2 = 0x20` | `DisplayNumberDeaths, O2, 0x00000010` | ✅ | +| `PlayerModule::SetUseCraftSuccessDialog @0x005D34D0` | `options_ \|= 0x80000000` | `arg2 = 0x1a` | `UseCraftSuccessDialog, O1, 0x80000000` | ✅ | +| `PlayerModule::ConfirmVolatileRareUse @0x005D3510` (getter) | `options2_ >> 0x12` (bit 18) | id `0x2D` | `ConfirmVolatileRareUse, O2, 0x00040000` | ✅ | + +Those five setter bodies also independently confirm two mechanisms the code +claims: the **unchanged-value early return** +(`if (((options >> bit) & 1) == arg2) return;`) and the **local write +BEFORE the `OnChanged` tail-jump** (`this->options_ = eax_1;` then +`jump vtable+0x14`). Both are exactly what `TrySetOption` models. + +The `CharacterOptionId` enum (`SocialActions.cs:362-417`) is likewise a +faithful transcription: every member is `_PlayerOption` with the +suffix dropped, except the six `Hear*Chat`→`ListenTo*Chat` renames the doc +comment itself declares, plus the ACE-sourced `HearPkDeathMessages`. + +--- + +## 2. Golden byte vector — recomputed independently, matches + +`tests/AcDream.Core.Net.Tests/Messages/SocialActionsTests.cs:141-198`. + +**Is it genuinely hand-computed?** Yes. `expected` is a literal `byte[]` +with per-line field comments; it is not produced by calling +`BuildSetCharacterOptions` or any helper. It is capable of catching the +CH3 failure mode (green tests pinning a wrong shape). + +**Does it encode the documented layout?** I recomputed all 92 bytes from +wire research §2.7 without looking at the builder: + +| # | Field | §2.7 rule | Expected bytes | Match | +|---|---|---|---|---| +| 1 | envelope | `u32 0xF7B1` | `B1 F7 00 00` | ✅ | +| 2 | seq | `u32 5` | `05 00 00 00` | ✅ | +| 3 | opcode | `u32 0x1A1` | `A1 01 00 00` | ✅ | +| 4 | header | `0x460 \| 0x01 \| 0x08` = `0x469` | `69 04 00 00` | ✅ | +| 5 | options1 | `0x50C4A54A` LE | `4A A5 C4 50` | ✅ | +| 6 | shortcut count | `u32 1` | `01 00 00 00` | ✅ | +| 7-9 | shortcut record | `i32 index, u32 objectID, u32 spellID` | `00…`, `01 00 00 80`, `00…` | ✅ | +| 10-11 | tab0 | `u32 1`, `u32 1234 (0x4D2)` | `01 00 00 00`, `D2 04 00 00` | ✅ | +| 12-18 | tabs 1..7 | 7 × lone `u32 0` | 7 × `00 00 00 00` | ✅ | +| 19 | desiredComps sizeInfo | `(tableSize<<16)\|count` = `1` | `01 00 00 00` | ✅ | +| 20-21 | one kv pair | `u32 key, u32 value` | `01 00 00 68`, `0C 00 00 00` | ✅ | +| 22 | spellbookFilters | `u32 0x3FFF` | `FF 3F 00 00` | ✅ | +| 23 | options2 | `u32 0x00948700` LE | `00 87 94 00` | ✅ | + +Length: 12 + 4 + 4 + 4 + 12 + 8 + 28 + 4 + 8 + 4 + 4 = **92** ✅ (asserted). +Payload 80 bytes ⇒ tail pad 0 ✅. + +**Builder conformance to §2.3–§2.7** (`SocialActions.cs:214-301`): + +- Four unconditional `u32`s in pack order (header, options1, …, + spellbookFilters, options2) ✅ — and the 16 unconditional payload bytes + match `PlayerModule::GetPackSize @0x005D4500`'s `mov esi, 0x10`. +- Header base `0x400|0x020|0x040 = 0x460` ✅ exactly per the `SetPackHeader + @0x005D44A0` disassembly (`SocialActions.cs:77-80`). +- Optional sections in retail's order (shortcuts → 8 lists → desired comps) + ✅ matches §2.3 field numbering 3/4/5. +- Never sets `0x100` ✅, omits `0x200` ✅ (only `|= 0x001` and `|= 0x008` + are reachable). +- Echo-not-zero ✅ — `CharacterOptionsBlobSource.Capture` pulls live + `Options1/Options2`, `Spellbook.GetFavorites(0..7)`, + `Spellbook.DesiredComponents`, `Spellbook.SpellbookFilters`, and + `Inventory.Shortcuts`, honouring §5.3's "do echo the real values anyway". +- 4-byte tail pad computed ✅ (`:245`), always 0 in practice, as the doc + comment states. +- Exactly-8-lists precondition enforced with a throw ✅ (`:226-231`). + +The `PlayerDescriptionParser` round-trip test is a *genuine* independent +oracle: that parser predates this builder and was written against ACE's +outbound `PlayerDescription`. Its read order (flags, options1, shortcuts +under `0x01`, 8 lists under `0x400`, desired comps under `0x08`, filters, +options2 under `0x40`) matches the builder's write order field-for-field. +The `u32 count` vs `u16 count + u16 pad` difference is equivalent on +little-endian for counts < 65536, exactly as §2.4c notes. + +--- + +## 3. Findings + +### MUST-FIX 1 — TS-71's stated deferral rationale asserts a constraint that does not exist + +**Where:** `docs/architecture/retail-divergence-register.md:353` ("Why +deferred" column), and the same claim in the commit message. + +The row says: + +> Wiring the timer requires a new per-frame phase touching both +> `AcDream.App`'s `UpdateFrameOrchestrator` graph and `AcDream.Headless`'s +> `HeadlessSessionHost.Tick` — outside Campaign OP slice OP1's +> Runtime/wire-layer scope. + +**That is not true.** Both hosts already funnel through ONE Runtime-owned +per-session tick: + +- Headless: `src/AcDream.Headless/Hosting/HeadlessSessionHost.cs:341` → + `Runtime.Session.Tick()` +- Graphical: `src/AcDream.App/World/RetailLiveFrameCoordinator.cs:61` → + `_session.Tick()`, where `_session` is `IRuntimeLiveSessionFramePhase` + (`:24`), i.e. Runtime's own `LiveSessionController.Tick()` + (`src/AcDream.Runtime/Session/LiveSessionController.cs:417`). + +`LiveSessionController` and its `ProductionLiveSessionOperations` +(`LiveSessionController.cs:137`) are both Runtime-internal and constructed +by `GameRuntime`, which owns `CharacterOwner`. Adding the +`TryFlushIfAutoSaveDue` call there is a **Runtime-internal change requiring +zero host edits** — no new per-frame phase, no `UpdateFrameOrchestrator` +graph change, no `HeadlessSessionHost.Tick` body change. + +The plan named this exact seam: +`docs/plans/2026-08-10-options-panel-campaign.md:157-158` — *"flush triggers += explicit `SaveOptions` command, session logout, and the 480 s timer +(retail constant, lane C §3.3) **driven from the existing Runtime tick**"* — +and set the target as "not deferred". + +This is the precise failure mode CLAUDE.md's C4-closeout handoff flags as +having cost the most that campaign: *a contract asserting a mechanism that +does not exist*, and *inferring a fact you can observe*. The register is +the auditable record; a future reader will act on that column. + +**Fix (either is acceptable):** wire `TryFlushIfAutoSaveDue` into +`LiveSessionController.Tick()` and retire the timer half of TS-71 — the +plan's stated target — or restate the row's "Why deferred" column +truthfully (e.g. "the flush needs a `WorldSession` + character-state pair +at the Runtime tick seam, which `LiveSessionController` does not currently +hold; deferred rather than widen its constructor mid-slice"). The logout +half's rationale is defensible as written and I am not challenging it. + +Everything else about TS-71 is accurate: the symptom description +("a player who toggles ONLY batched options and then disconnects without +pressing Apply loses those toggles"), the scope ("auto-save ids are +unaffected"), the file pointer, and both retail anchors +(`CPlayerModule::UseTime @0x0059A710`, `CPlayerSystem::LogOffCharacter +@0x00563520`) check out against §3.3. + +### SHOULD-FIX 2 — `TrySetOption` omits `OnChanged`'s side-effect switch while claiming to mirror it "exactly" + +**Where:** `src/AcDream.Runtime/Gameplay/RuntimeCharacterState.cs:696` +(doc comment) and `:712-730` (body). + +The doc comment says it *"Mirrors `CPlayerModule::OnChanged(PlayerOption) +@0x0059A8E0` **exactly**"*. I read that function's body in the decomp. It +has four steps; `TrySetOption` implements two: + +| Retail step | In `TrySetOption`? | +|---|---| +| 1. `CM_UI::SendNotice_PlayerOptionChanged(opt)` | ❌ (acceptable — `Revision` is the local fan-out; no consumer needs more today) | +| 2. six-case side-effect switch | ❌ **not modelled at all** | +| 3. `if (IsAutoSaveOption) { Event_PlayerOptionChangedEvent(...); return; }` | ✅ | +| 4. `else if (!m_bDirty) { m_bDirty = 1; m_timeFirstDirtied = cur_time; }` | ✅ | + +Four of step 2's six cases (`0x04` weather, `0x05` day, `0x07` combat +target, `0x30` fog) are presentation bindings the campaign plan defers to +later slices — fine. **The other two are state mutations and belong with +the option owner:** + +``` +case 2: if (IgnoreFellowshipRequests(this)) SetFellowshipAutoAcceptRequests(this, 0); +case 0x12: if (FellowshipAutoAcceptRequests(this)) SetIgnoreFellowshipRequests(this, 0); +``` + +Both `0x02` and `0x12` are auto-save ids. Because those are recursive +`SetXxx` calls, retail runs the cleared option through its own accessor → +`OnChanged` → `IsAutoSaveOption` → **a second `0x0005` on the wire**. So +turning on either fellowship option while the other is set produces *two* +messages in retail and leaves exactly one bit set. acdream sends one and +leaves **both** bits set — a state retail cannot produce, and one ACE +stores without complaint (§5.1: no validation, no clamping). ACE's +`Entity/Fellowship.cs:98` checks the ignore flag first, so the auto-accept +bit would be silently dead. + +**Reachability today: none.** No acdream entrance can set `0x02`/`0x12` — +the only live callers are the six `ListenTo*Chat` ids +(`ClientCommandController.cs:287,292` and the Settings chat toggles), and +the headless `characterOptions` block does not exist yet. So this is +latent, not a live bug. + +**But it becomes live the moment OP4 ships the Character tab**, and the +plan makes `TrySetOption` the single seam that tab writes through. There is +no register row and no plan note: `grep` for `OnChanged` and +`FellowshipAutoAccept` across `retail-divergence-register.md` and +`2026-08-10-options-panel-campaign.md` returns **0 hits**. + +**Fix:** either implement the two mutual-exclusion cases inside +`TrySetOption` (they compose naturally — a recursive `TrySetOption(other, +false, sendAutoSave')` call reproduces retail's second `0x0005`), or add a +register row and soften the doc comment from "exactly" to name what is and +is not ported. Note the research's own planner section (§7.1 Group A) files +these two ids under "wire + state only, ACE does the rest", which +under-states them — §1.5 and §3.1 are the authoritative sections and both +document the mechanism. + +### SHOULD-FIX 3 — no test pins the id→(word, mask) map + +The auto-save and client-default columns are pinned beautifully: hand- +transcribed id arrays (`CharacterOptionTableTests.cs:20-43` and `:50-68`) +driven against a `[Theory]` that walks all 53 ids. Both arrays match the +byte tables exactly; this is the right shape. + +**The word/mask column has no equivalent.** It is covered only by: + +- `SpotCheck_WordAndMaskAgainstVerbatimAcclientEnums` (`:152-181`) — 4 ids. +- `ReconstructedClientDefaultWords_MatchIndependentlyConfirmedConstants` + (`:119-140`) — constrains the *set* of the 16 `ClientDefault` masks, not + the id→mask assignment, and says nothing about the other 37. + +So a transposition among the 37 non-default masks — swapping `DisplayAge` +(`O2 0x20`) with `DisplayNumberDeaths` (`O2 0x10`), say — passes the entire +suite silently. That is exactly the REJECT-class data this review was asked +to guard, and it is the one column with no id-by-id pin. + +I verified all 53 by hand this round, so nothing is wrong *today*. Add a +53-row `[InlineData(id, isOptions1, mask)]` theory transcribed from +`acclient.h` so the next edit cannot silently alter one. + +### NOTE 4 — header bit predicate: `Count > 0` vs retail's pointer-non-null + +`SocialActions.cs:234-235` sets `0x001`/`0x008` when the collection is +non-empty. `PlayerModule::SetPackHeader @0x005D44A0` tests the *member +pointer* (`test eax,eax` on `shortcuts_`, `[ecx+0x88]` on +`desired_comps_`), so a retail client with an allocated-but-empty +`ShortCutManager` still sets `0x01` and emits `u32 count = 0`. Per §2.5, +an absent flag means the receiver *destroys* its copy rather than emptying +it. Unobservable against ACE (both sections are read-and-discarded, §5.3), +and the code comment honestly says "non-empty" rather than claiming the +retail predicate. No action; recorded so a retail-server target does not +inherit it unexamined. + +### NOTE 5 — `desiredComps` sizeInfo high half written as 0 + +Retail packs `(_table_size << 16) | _currNum` +(`PackableHashTable::Pack @0x005692B0`); acdream writes the count alone. +Matches §2.4c ("advisory; ACE ignores it") and is commented at +`SocialActions.cs:277-279`. No action. + +### NOTE 6 — flush delegates run under `_dirtyGate` + +`TryFlush` (`RuntimeCharacterState.cs:789-799`) and +`TryFlushIfAutoSaveDue` (`:808-819`) invoke the caller's `flush` action +while holding the lock, then clear `_isDirty`. A re-entrant `MarkDirty` +from inside `flush` would succeed (C# locks are reentrant) and then be +erased by the trailing `_isDirty = false`. Not reachable from either +adapter's current closure. Worth knowing before the Options panel starts +flushing from inside change handlers. + +### NOTE 7 — `TrySetOption`'s compare-and-write is not atomic + +`:718-722` reads the word via a `Volatile.Read` property, compares, then +calls `SetOptionBit`, which re-reads. Fine under the binding +one-update-thread Runtime contract (memory `#368`); flagged only so nobody +later assumes it is thread-safe. + +### NOTE 8 — `SaveOptions` result asymmetry between hosts + +`DirectGameRuntimeCommandAdapter.SaveOptions` reports whether the flush +actually fired (`primaryObjectId: flushed ? 1u : 0u`); +`CurrentGameRuntimeCommandAdapter.SaveOptions` publishes a bus command and +always reports `Accepted`, because `LiveCommandBus.Publish` has no return +channel. Inherent to the graphical bus (the same reason the adapter +pre-validates the option id at `:681-687`), not a defect. + +### NOTE 9 — comment wording at `SocialActions.cs:65` + +"The other **unconditional** bits from the same disassembly are OR'd in +below when their section is non-empty" — `0x001`/`0x008` are the +*conditional* bits. One-word fix. + +### NOTE 10 — the factory closure survived, correctly + +The plan said "The `LiveSessionRuntimeFactory` local closure is deleted, +not duplicated". `SendSingleCharacterOption` still exists +(`LiveSessionRuntimeFactory.cs:347-352`) — it must, to bind `session` — +but it now holds no policy, only the `sendAutoSave` binding. The plan's +intent (one policy, one place) is satisfied. + +--- + +## 4. Items that passed cleanly + +**Q3 — routing and rejection at every wire-reaching seam.** ✅ + +`grep` across `src/` finds exactly **two** call sites of +`WorldSession.SendSetSingleCharacterOption` outside `WorldSession` itself: +`LiveSessionRuntimeFactory.cs:352` and +`DirectGameRuntimeCommandAdapter.cs:670`. **Both are inside a +`TrySetOption(...)` `sendAutoSave` closure**, so an id outside +`CharacterOptionTable` — including the `0x35`/`0x36` landmines that would +OR a whole default mask into ACE's word (§5.4.3) — physically cannot reach +the wire. `CurrentGameRuntimeCommandAdapter` additionally pre-rejects at +`:681-687` because the bus cannot carry a rejection back. Both `App` +entrances (`ClientCommandController.Bindings.SetSingleCharacterOption` at +`:287,292` and `RuntimeSettingsTargets.SetSingleCharacterOption` at `:321`) +converge on the same local function. `SetOptionBit`'s remaining direct +callers are the state class itself and tests only. + +Ordering is retail's: local write first (`SetOptionBit` at `:722`), then +send-or-dirty (`:724-727`) — matching the setter bodies I read +(`this->options_ = eax_1;` then `jump vtable+0x14`). The headless +local-write gap that lanes B §4.4 and C §7.4 independently found is closed. + +**Q4 — flush semantics vs §3.3–§3.5.** ✅ The pure logic is correct. + +- `MarkDirty` (`:773-781`) stamps only on the *first* dirtying change — + matches `if (!m_bDirty) { m_bDirty = 1; m_timeFirstDirtied = cur_time; }`. +- Auto-save ids never dirty (`:724-727` is an if/else) — matches step 3's + `return`. +- `TryFlush` no-ops when clean — matches `SaveToServer(force: 0)` with both + production call sites passing 0. +- `TryFlushIfAutoSaveDue` fires at `elapsed >= 480 s` — matches the + byte-verified `480.0 + m_timeFirstDirtied <= Timer::cur_time`, *including + the inclusive boundary*, and the test pins 479 s → no / 480 s → yes. +- Unchanged value produces nothing at all (`:719-720`) — matches §3.5's + last row and the accessors' own early return. + +Nothing else is silently unwired: `SaveOptions` is implemented on both +production `IRuntimeCharacterCommands` implementors and registered on the +graphical router (`SaveCharacterOptionsRuntimeCmd`); the only other +implementor is a test double. + +**Q5 — retail anchors.** ✅ Every ported mechanism cites a named symbol + +address, and I resolved **15/15** cited addresses in +`docs/research/named-retail/symbols.json`; every symbol↔address pair is +exactly as claimed: + +| Cited in code / register | symbols.json | +|---|---| +| `CPlayerModule::IsAutoSaveOption @0x0059A600` | ✅ | +| `PlayerModule::GetDefaultOptionValue @0x005D2A30` | ✅ | +| `CPlayerModule::UseTime @0x0059A710` | ✅ | +| `CPlayerModule::OnChanged @0x0059A8E0` | ✅ | +| `CPlayerModule::SaveToServer @0x0059A660` | ✅ | +| `PlayerModule::SetHearGeneralChat @0x005D35C0` | ✅ | +| `PlayerModule::Pack @0x005D45C0` | ✅ | +| `PlayerModule::SetPackHeader @0x005D44A0` | ✅ | +| `PlayerModule::GetPackSize @0x005D4500` | ✅ | +| `PackableHashTable::Pack @0x005692B0` | ✅ | +| `PlayerModule::PlayerModule @0x005D51F0` | ✅ | +| `CPlayerSystem::LogOffCharacter @0x00563520` | ✅ | +| `CM_Character::Event_CharacterOptionsEvent @0x006A10C0` | ✅ | +| `PlayerModule::GetOption @0x005D3AA0` | ✅ | +| `PlayerModule::SetOption @0x005D3EB0` | ✅ | + +Header line-range citations also check out exactly: +`acclient.h:3404-3436` (`CharacterOption` spans 3404→3436), +`:3451-3481` (`CharacterOptions2`), `:4162-4218` (`PlayerOption`, +terminating at `TotalNumberOfPlayerOptions = 0x34` on line 4217), +`:36507` (`struct PlayerModule`), `:7835` (`PlayerModulePackHeader`). + +**AP-193 and AP-194.** ✅ Both accurately describe what they cover. +AP-193 correctly scopes the `0x34` row as ACE-sourced and unverifiable, and +correctly notes the 2013 bounds check (`cmp eax, 0x33 / ja`) is evidence +about the 2013 build, not the shipping one — matching §8.1 U4. AP-194 +correctly states the `GetDefaultOptionValue` vs constructor-default +disagreement for `0x2D`/`0x2F`/`0x32`, correctly attributes it to the +table's `0x2A` bound, and correctly instructs future readers not to "fix" +it — matching §8.2. The code reproduces the quirk rather than correcting +it, which is the right call. + +--- + +## 5. Verdict + +**APPROVE-WITH-FIXES.** + +The transcription work is excellent — all 53 rows are right, the golden +vector is right and genuinely independent, the wire layout matches §2.3–§2.7 +field for field, the flush state machine matches §3.3–§3.5 including the +480 s boundary, and unknown/`0x35`/`0x36` ids cannot reach the wire from +any seam. The two substantive gaps are that a declared deviation's stated +*cause* is false (MUST-FIX 1) and that the retail function the shared seam +claims to mirror "exactly" is only half ported, with no register row +(SHOULD-FIX 2). Neither blocks the slice's data; both must be closed before +OP4 puts a Character tab on top of this seam.