# 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.