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>
29 KiB
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— updatedsrc/AcDream.App/Runtime/CurrentGameRuntimeCommandAdapter.cs:28— updatedtests/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 generalwhen already listening now sends nothing (was: a redundant0x0005).- A Settings save whose persisted
ChatSettingsdiff 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:272Advance(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 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
SetOptionBitoutsideTrySetOption. - No
switch/Enum.GetValuesexhaustiveness dependency onCharacterOptionId. RetailClientCommandCatalog.JoinLeaveTags' six hard-coded ids all still map to auto-save rows, so@join/@leavestill sends0x0005.LiveCommandBus.Publishis synchronous — no reorder window between a toggle and a flush.SaveCharacterOptionsRuntimeCmdis generation/activity gated identically to every other router command (SendIfActive), pinned byLiveSessionCommandRouterTests.cs:444-456.Characterdomain operation ids:Advanceuses(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.
TryFlushleaves the module dirty if the send throws (correct direction).SaveCharacterOptionsIfDirtycaptures the per-sessionWorldSessionpassed intoCreateCommandBindings, so no stale-session capture across generations.- All 53 table masks are pairwise distinct;
ClientDefaultreconstruction reproduces0x50C4A54A/0x00008700; the constructor default0x00948700decomposes 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.