acdream/docs/research/2026-08-10-op1-review-blast.md

# 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<CharacterOptionId>()`
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
`<see cref="AcDream.Runtime.Gameplay.CharacterOptionTable"/>` from
`AcDream.Core.Net`, which does not (and must not) reference `AcDream.Runtime`.
`:201` uses `<see cref="favoriteSpells"/>` 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.