acdream/docs/research/2026-08-10-op1-review-blast.md
Erik 0df0a60424 docs: OP1 dual-lens review findings — two APPROVE-WITH-FIXES
Mechanism lens: 53/53 table rows verified four independent ways; golden
vector recomputed byte-for-byte; 15/15 citations resolve. MUST-FIX: TS-71's
deferral rationale asserts a nonexistent obstacle — both hosts already
funnel one Runtime tick seam (LiveSessionController.Tick), so the 480 s
timer + logout flush wire with zero host edits. SHOULD-FIX: port
CPlayerModule::OnChanged cases 2/0x12 (fellowship mutual-exclusion emits a
second 0x0005); add the id-by-id 53-row word/mask pin.

Blast lens: CH3/CH4 seams bit-identical; routes single-write; reset clean;
the blob echo reads canonical J4.3/J4.5 owners (the important negative).
MUST-FIX: SaveOptions before the PlayerDescription seed would flush CLIENT
DEFAULTS over server options — needs a HasServerSeed latch (set by
Replace, required by TryFlush, cleared by ResetSession). SHOULD-FIX: router
test substitutes a fake binding for the production seam; flush callback
runs under _dirtyGate (deadlock with the router gate once the timer
wires); ledger blind to IsDirty; SaveOptions result encoding differs
between adapters; Replace leaves stale dirty state; no cross-check between
PlayerDescriptionParser enums and CharacterOptionTable.

Fix round follows as one consolidated pass.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-08-10 23:48:11 +02:00

29 KiB
Raw Blame History

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 SetOptionBitTrySetOption 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) → SetSingleCharacterOptionRuntimeCmdLiveSessionCommandRouter (:173-177) → bindings.SendSingleCharacterOptionTrySetOption one local write, then one 0x0005unchanged 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.ItemsRuntimeInventoryState.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 crefs 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 = 03 (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.