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

Campaign OP slice OP1 — mechanism-faithfulness review

Commit under review: 86c0a7e0 ("feat(runtime,net): Campaign OP slice OP1 — full character-option table, dirty model, real 0x01A1 blob builder"). Lens: retail-mechanism faithfulness and contract fidelity ONLY. Blast radius is a sibling reviewer's lane. Mode: read-only. No build, no test run, no launch. Git reads only. Date: 2026-08-10.

Verdict: APPROVE-WITH-FIXES. 1 MUST-FIX, 2 SHOULD-FIX, 7 NOTE.

The ported data is correct. I verified all 53 table rows (not a sample) against the verbatim retail header, re-decoded both byte tables from the research myself, and recomputed the golden wire vector byte-for-byte — every one matches. The defects are (a) a register row whose stated reason for a deferral is factually wrong, and (b) an omitted half of the retail function the code claims to mirror "exactly".

---

1. Row-by-row table check — 53 / 53 rows verified

src/AcDream.Runtime/Gameplay/CharacterOptionTable.cs:111-165.

Method. I did not trust the research doc's summary tables. For each of the 53 rows I checked four independent things:

1. id → name against docs/research/named-retail/acclient.h:4162-4218 (enum PlayerOption, read directly).
2. name → word + mask against acclient.h:3404-3436 (enum CharacterOption = the Options1 bitfield) and acclient.h:3451-3481 (enum CharacterOptions2), read directly.
3. IsAutoSave against my own decode of the 0x34-byte jump table quoted at docs/research/2026-08-10-set-character-options-wire.md §3.2 — I indexed the 52 bytes myself rather than reading off the doc's id list.
4. ClientDefault against my own decode of the 0x2B-byte table quoted at the same doc's §8.2 / 2026-08-10-character-options-map.md §1.4.

Result: 53/53 rows correct. Zero altered ids, zero altered masks, zero altered flags. Specifically:

Column	Expected	Found in code	Verdict
id coverage	contiguous 0x00..0x34	contiguous 0x00..0x34, 53 entries	✅
word + mask	53 rows from acclient.h	53 rows	✅ all 53
IsAutoSave	21 ids: 00 01 02 0F 10 11 12 19 1B 23 24 25 26 27 2A 2B 2E 2F 31 32 33	identical set, count 21	✅
ClientDefault	16 ids: 00 02 06 08 0A 0D 0E 0F 14 15 19 1B 23 24 25 2A	identical set, count 16	✅

My own decode of the §3.2 table (00⇒auto-save) yields exactly the 21 ids above; my own decode of the §8.2 table (43 bytes, indices 0x00..0x2A, 00⇒true) yields exactly the 16 above, with 0x2B..0x34 falling off the end to false. Both match the code.

Independent arithmetic cross-check. OR-ing every ClientDefault=true row's mask into its own word by hand:

• Options1 = 0x02|0x08|0x40|0x100|0x400|0x2000|0x8000|0x40000|0x400000|0x800000|0x10000000|0x40000000 = 0x50C4A54A ✅ (matches the byte-verified constructor literal at PlayerModule::PlayerModule @0x005D51F0)
• Options2 = 0x100|0x200|0x400|0x8000 = 0x00008700 ✅

This is a genuine cross-confirmation of the id→mask assignment for those 16 rows, exactly as the type's XML doc claims (CharacterOptionTable.cs:41-45).

Deeper verification of the five highest-risk rows. The rows where a transposition would be invisible are the ones whose mask is out of order relative to the id. I read their retail setter bodies directly out of docs/research/named-retail/acclient_2013_pseudo_c.txt rather than relying on the header:

Retail setter	Writes	Tail-jumps with id	Code row	Verdict
PlayerModule::SetIgnoreTradeRequests @0x005D2B90	options_ |= 0x20000	arg2 = 3	IgnoreTradeRequests, O1, 0x00020000	✅
PlayerModule::SetDisplayAge @0x005D37E0	options2_ |= 0x20	arg2 = 0x1d	DisplayAge, O2, 0x00000020	✅
PlayerModule::SetDisplayNumberDeaths @0x005D38D0	options2_ |= 0x10	arg2 = 0x20	DisplayNumberDeaths, O2, 0x00000010	✅
PlayerModule::SetUseCraftSuccessDialog @0x005D34D0	options_ |= 0x80000000	arg2 = 0x1a	UseCraftSuccessDialog, O1, 0x80000000	✅
PlayerModule::ConfirmVolatileRareUse @0x005D3510 (getter)	options2_ >> 0x12 (bit 18)	id 0x2D	ConfirmVolatileRareUse, O2, 0x00040000	✅

Those five setter bodies also independently confirm two mechanisms the code claims: the unchanged-value early return (if (((options >> bit) & 1) == arg2) return;) and the local write BEFORE the OnChanged tail-jump (this->options_ = eax_1; then jump vtable+0x14). Both are exactly what TrySetOption models.

The CharacterOptionId enum (SocialActions.cs:362-417) is likewise a faithful transcription: every member is <Name>_PlayerOption with the suffix dropped, except the six Hear*Chat→ListenTo*Chat renames the doc comment itself declares, plus the ACE-sourced HearPkDeathMessages.

---

2. Golden byte vector — recomputed independently, matches

tests/AcDream.Core.Net.Tests/Messages/SocialActionsTests.cs:141-198.

Is it genuinely hand-computed? Yes. expected is a literal byte[] with per-line field comments; it is not produced by calling BuildSetCharacterOptions or any helper. It is capable of catching the CH3 failure mode (green tests pinning a wrong shape).

Does it encode the documented layout? I recomputed all 92 bytes from wire research §2.7 without looking at the builder:

#	Field	§2.7 rule	Expected bytes	Match
1	envelope	u32 0xF7B1	B1 F7 00 00	✅
2	seq	u32 5	05 00 00 00	✅
3	opcode	u32 0x1A1	A1 01 00 00	✅
4	header	0x460 | 0x01 | 0x08 = 0x469	69 04 00 00	✅
5	options1	0x50C4A54A LE	4A A5 C4 50	✅
6	shortcut count	u32 1	01 00 00 00	✅
7-9	shortcut record	i32 index, u32 objectID, u32 spellID	00…, 01 00 00 80, 00…	✅
10-11	tab0	u32 1, u32 1234 (0x4D2)	01 00 00 00, D2 04 00 00	✅
12-18	tabs 1..7	7 × lone u32 0	7 × 00 00 00 00	✅
19	desiredComps sizeInfo	(tableSize<<16)|count = 1	01 00 00 00	✅
20-21	one kv pair	u32 key, u32 value	01 00 00 68, 0C 00 00 00	✅
22	spellbookFilters	u32 0x3FFF	FF 3F 00 00	✅
23	options2	u32 0x00948700 LE	00 87 94 00	✅

Length: 12 + 4 + 4 + 4 + 12 + 8 + 28 + 4 + 8 + 4 + 4 = 92 ✅ (asserted). Payload 80 bytes ⇒ tail pad 0 ✅.

Builder conformance to §2.3–§2.7 (SocialActions.cs:214-301):

• Four unconditional u32s in pack order (header, options1, …, spellbookFilters, options2) ✅ — and the 16 unconditional payload bytes match PlayerModule::GetPackSize @0x005D4500's mov esi, 0x10.
• Header base 0x400|0x020|0x040 = 0x460 ✅ exactly per the SetPackHeader @0x005D44A0 disassembly (SocialActions.cs:77-80).
• Optional sections in retail's order (shortcuts → 8 lists → desired comps) ✅ matches §2.3 field numbering 3/4/5.
• Never sets 0x100 ✅, omits 0x200 ✅ (only |= 0x001 and |= 0x008 are reachable).
• Echo-not-zero ✅ — CharacterOptionsBlobSource.Capture pulls live Options1/Options2, Spellbook.GetFavorites(0..7), Spellbook.DesiredComponents, Spellbook.SpellbookFilters, and Inventory.Shortcuts, honouring §5.3's "do echo the real values anyway".
• 4-byte tail pad computed ✅ (:245), always 0 in practice, as the doc comment states.
• Exactly-8-lists precondition enforced with a throw ✅ (:226-231).

The PlayerDescriptionParser round-trip test is a genuine independent oracle: that parser predates this builder and was written against ACE's outbound PlayerDescription. Its read order (flags, options1, shortcuts under 0x01, 8 lists under 0x400, desired comps under 0x08, filters, options2 under 0x40) matches the builder's write order field-for-field. The u32 count vs u16 count + u16 pad difference is equivalent on little-endian for counts < 65536, exactly as §2.4c notes.

---

3. Findings

MUST-FIX 1 — TS-71's stated deferral rationale asserts a constraint that does not exist

Where: docs/architecture/retail-divergence-register.md:353 ("Why deferred" column), and the same claim in the commit message.

The row says:

Wiring the timer requires a new per-frame phase touching both AcDream.App's UpdateFrameOrchestrator graph and AcDream.Headless's HeadlessSessionHost.Tick — outside Campaign OP slice OP1's Runtime/wire-layer scope.

That is not true. Both hosts already funnel through ONE Runtime-owned per-session tick:

• Headless: src/AcDream.Headless/Hosting/HeadlessSessionHost.cs:341 → Runtime.Session.Tick()
• Graphical: src/AcDream.App/World/RetailLiveFrameCoordinator.cs:61 → _session.Tick(), where _session is IRuntimeLiveSessionFramePhase (:24), i.e. Runtime's own LiveSessionController.Tick() (src/AcDream.Runtime/Session/LiveSessionController.cs:417).

LiveSessionController and its ProductionLiveSessionOperations (LiveSessionController.cs:137) are both Runtime-internal and constructed by GameRuntime, which owns CharacterOwner. Adding the TryFlushIfAutoSaveDue call there is a Runtime-internal change requiring zero host edits — no new per-frame phase, no UpdateFrameOrchestrator graph change, no HeadlessSessionHost.Tick body change.

The plan named this exact seam: docs/plans/2026-08-10-options-panel-campaign.md:157-158 — "flush triggers = explicit SaveOptions command, session logout, and the 480 s timer (retail constant, lane C §3.3) driven from the existing Runtime tick" — and set the target as "not deferred".

This is the precise failure mode CLAUDE.md's C4-closeout handoff flags as having cost the most that campaign: a contract asserting a mechanism that does not exist, and inferring a fact you can observe. The register is the auditable record; a future reader will act on that column.

Fix (either is acceptable): wire TryFlushIfAutoSaveDue into LiveSessionController.Tick() and retire the timer half of TS-71 — the plan's stated target — or restate the row's "Why deferred" column truthfully (e.g. "the flush needs a WorldSession + character-state pair at the Runtime tick seam, which LiveSessionController does not currently hold; deferred rather than widen its constructor mid-slice"). The logout half's rationale is defensible as written and I am not challenging it.

Everything else about TS-71 is accurate: the symptom description ("a player who toggles ONLY batched options and then disconnects without pressing Apply loses those toggles"), the scope ("auto-save ids are unaffected"), the file pointer, and both retail anchors (CPlayerModule::UseTime @0x0059A710, CPlayerSystem::LogOffCharacter @0x00563520) check out against §3.3.

SHOULD-FIX 2 — TrySetOption omits OnChanged's side-effect switch while claiming to mirror it "exactly"

Where: src/AcDream.Runtime/Gameplay/RuntimeCharacterState.cs:696 (doc comment) and :712-730 (body).

The doc comment says it "Mirrors CPlayerModule::OnChanged(PlayerOption) @0x0059A8E0 exactly". I read that function's body in the decomp. It has four steps; TrySetOption implements two:

Retail step	In TrySetOption?
1. CM_UI::SendNotice_PlayerOptionChanged(opt)	❌ (acceptable — Revision is the local fan-out; no consumer needs more today)
2. six-case side-effect switch	❌ not modelled at all
3. if (IsAutoSaveOption) { Event_PlayerOptionChangedEvent(...); return; }	✅
4. else if (!m_bDirty) { m_bDirty = 1; m_timeFirstDirtied = cur_time; }	✅

Four of step 2's six cases (0x04 weather, 0x05 day, 0x07 combat target, 0x30 fog) are presentation bindings the campaign plan defers to later slices — fine. The other two are state mutations and belong with the option owner:

case 2:     if (IgnoreFellowshipRequests(this))    SetFellowshipAutoAcceptRequests(this, 0);
case 0x12:  if (FellowshipAutoAcceptRequests(this)) SetIgnoreFellowshipRequests(this, 0);

Both 0x02 and 0x12 are auto-save ids. Because those are recursive SetXxx calls, retail runs the cleared option through its own accessor → OnChanged → IsAutoSaveOption → a second 0x0005 on the wire. So turning on either fellowship option while the other is set produces two messages in retail and leaves exactly one bit set. acdream sends one and leaves both bits set — a state retail cannot produce, and one ACE stores without complaint (§5.1: no validation, no clamping). ACE's Entity/Fellowship.cs:98 checks the ignore flag first, so the auto-accept bit would be silently dead.

Reachability today: none. No acdream entrance can set 0x02/0x12 — the only live callers are the six ListenTo*Chat ids (ClientCommandController.cs:287,292 and the Settings chat toggles), and the headless characterOptions block does not exist yet. So this is latent, not a live bug.

But it becomes live the moment OP4 ships the Character tab, and the plan makes TrySetOption the single seam that tab writes through. There is no register row and no plan note: grep for OnChanged and FellowshipAutoAccept across retail-divergence-register.md and 2026-08-10-options-panel-campaign.md returns 0 hits.

Fix: either implement the two mutual-exclusion cases inside TrySetOption (they compose naturally — a recursive TrySetOption(other, false, sendAutoSave') call reproduces retail's second 0x0005), or add a register row and soften the doc comment from "exactly" to name what is and is not ported. Note the research's own planner section (§7.1 Group A) files these two ids under "wire + state only, ACE does the rest", which under-states them — §1.5 and §3.1 are the authoritative sections and both document the mechanism.

SHOULD-FIX 3 — no test pins the id→(word, mask) map

The auto-save and client-default columns are pinned beautifully: hand- transcribed id arrays (CharacterOptionTableTests.cs:20-43 and :50-68) driven against a [Theory] that walks all 53 ids. Both arrays match the byte tables exactly; this is the right shape.

The word/mask column has no equivalent. It is covered only by:

• SpotCheck_WordAndMaskAgainstVerbatimAcclientEnums (:152-181) — 4 ids.
• ReconstructedClientDefaultWords_MatchIndependentlyConfirmedConstants (:119-140) — constrains the set of the 16 ClientDefault masks, not the id→mask assignment, and says nothing about the other 37.

So a transposition among the 37 non-default masks — swapping DisplayAge (O2 0x20) with DisplayNumberDeaths (O2 0x10), say — passes the entire suite silently. That is exactly the REJECT-class data this review was asked to guard, and it is the one column with no id-by-id pin.

I verified all 53 by hand this round, so nothing is wrong today. Add a 53-row [InlineData(id, isOptions1, mask)] theory transcribed from acclient.h so the next edit cannot silently alter one.

NOTE 4 — header bit predicate: Count > 0 vs retail's pointer-non-null

SocialActions.cs:234-235 sets 0x001/0x008 when the collection is non-empty. PlayerModule::SetPackHeader @0x005D44A0 tests the member pointer (test eax,eax on shortcuts_, [ecx+0x88] on desired_comps_), so a retail client with an allocated-but-empty ShortCutManager still sets 0x01 and emits u32 count = 0. Per §2.5, an absent flag means the receiver destroys its copy rather than emptying it. Unobservable against ACE (both sections are read-and-discarded, §5.3), and the code comment honestly says "non-empty" rather than claiming the retail predicate. No action; recorded so a retail-server target does not inherit it unexamined.

NOTE 5 — desiredComps sizeInfo high half written as 0

Retail packs (_table_size << 16) | _currNum (PackableHashTable::Pack @0x005692B0); acdream writes the count alone. Matches §2.4c ("advisory; ACE ignores it") and is commented at SocialActions.cs:277-279. No action.

NOTE 6 — flush delegates run under _dirtyGate

TryFlush (RuntimeCharacterState.cs:789-799) and TryFlushIfAutoSaveDue (:808-819) invoke the caller's flush action while holding the lock, then clear _isDirty. A re-entrant MarkDirty from inside flush would succeed (C# locks are reentrant) and then be erased by the trailing _isDirty = false. Not reachable from either adapter's current closure. Worth knowing before the Options panel starts flushing from inside change handlers.

NOTE 7 — TrySetOption's compare-and-write is not atomic

:718-722 reads the word via a Volatile.Read property, compares, then calls SetOptionBit, which re-reads. Fine under the binding one-update-thread Runtime contract (memory #368); flagged only so nobody later assumes it is thread-safe.

NOTE 8 — SaveOptions result asymmetry between hosts

DirectGameRuntimeCommandAdapter.SaveOptions reports whether the flush actually fired (primaryObjectId: flushed ? 1u : 0u); CurrentGameRuntimeCommandAdapter.SaveOptions publishes a bus command and always reports Accepted, because LiveCommandBus.Publish has no return channel. Inherent to the graphical bus (the same reason the adapter pre-validates the option id at :681-687), not a defect.

NOTE 9 — comment wording at SocialActions.cs:65

"The other unconditional bits from the same disassembly are OR'd in below when their section is non-empty" — 0x001/0x008 are the conditional bits. One-word fix.

NOTE 10 — the factory closure survived, correctly

The plan said "The LiveSessionRuntimeFactory local closure is deleted, not duplicated". SendSingleCharacterOption still exists (LiveSessionRuntimeFactory.cs:347-352) — it must, to bind session — but it now holds no policy, only the sendAutoSave binding. The plan's intent (one policy, one place) is satisfied.

---

4. Items that passed cleanly

Q3 — routing and rejection at every wire-reaching seam. ✅

grep across src/ finds exactly two call sites of WorldSession.SendSetSingleCharacterOption outside WorldSession itself: LiveSessionRuntimeFactory.cs:352 and DirectGameRuntimeCommandAdapter.cs:670. Both are inside a TrySetOption(...) sendAutoSave closure, so an id outside CharacterOptionTable — including the 0x35/0x36 landmines that would OR a whole default mask into ACE's word (§5.4.3) — physically cannot reach the wire. CurrentGameRuntimeCommandAdapter additionally pre-rejects at :681-687 because the bus cannot carry a rejection back. Both App entrances (ClientCommandController.Bindings.SetSingleCharacterOption at :287,292 and RuntimeSettingsTargets.SetSingleCharacterOption at :321) converge on the same local function. SetOptionBit's remaining direct callers are the state class itself and tests only.

Ordering is retail's: local write first (SetOptionBit at :722), then send-or-dirty (:724-727) — matching the setter bodies I read (this->options_ = eax_1; then jump vtable+0x14). The headless local-write gap that lanes B §4.4 and C §7.4 independently found is closed.

Q4 — flush semantics vs §3.3–§3.5. ✅ The pure logic is correct.

• MarkDirty (:773-781) stamps only on the first dirtying change — matches if (!m_bDirty) { m_bDirty = 1; m_timeFirstDirtied = cur_time; }.
• Auto-save ids never dirty (:724-727 is an if/else) — matches step 3's return.
• TryFlush no-ops when clean — matches SaveToServer(force: 0) with both production call sites passing 0.
• TryFlushIfAutoSaveDue fires at elapsed >= 480 s — matches the byte-verified 480.0 + m_timeFirstDirtied <= Timer::cur_time, including the inclusive boundary, and the test pins 479 s → no / 480 s → yes.
• Unchanged value produces nothing at all (:719-720) — matches §3.5's last row and the accessors' own early return.

Nothing else is silently unwired: SaveOptions is implemented on both production IRuntimeCharacterCommands implementors and registered on the graphical router (SaveCharacterOptionsRuntimeCmd); the only other implementor is a test double.

Q5 — retail anchors. ✅ Every ported mechanism cites a named symbol + address, and I resolved 15/15 cited addresses in docs/research/named-retail/symbols.json; every symbol↔address pair is exactly as claimed:

Cited in code / register	symbols.json
CPlayerModule::IsAutoSaveOption @0x0059A600	✅
PlayerModule::GetDefaultOptionValue @0x005D2A30	✅
CPlayerModule::UseTime @0x0059A710	✅
CPlayerModule::OnChanged @0x0059A8E0	✅
CPlayerModule::SaveToServer @0x0059A660	✅
PlayerModule::SetHearGeneralChat @0x005D35C0	✅
PlayerModule::Pack @0x005D45C0	✅
PlayerModule::SetPackHeader @0x005D44A0	✅
PlayerModule::GetPackSize @0x005D4500	✅
PackableHashTable::Pack @0x005692B0	✅
PlayerModule::PlayerModule @0x005D51F0	✅
CPlayerSystem::LogOffCharacter @0x00563520	✅
CM_Character::Event_CharacterOptionsEvent @0x006A10C0	✅
PlayerModule::GetOption @0x005D3AA0	✅
PlayerModule::SetOption @0x005D3EB0	✅

Header line-range citations also check out exactly: acclient.h:3404-3436 (CharacterOption spans 3404→3436), :3451-3481 (CharacterOptions2), :4162-4218 (PlayerOption, terminating at TotalNumberOfPlayerOptions = 0x34 on line 4217), :36507 (struct PlayerModule), :7835 (PlayerModulePackHeader).

AP-193 and AP-194. ✅ Both accurately describe what they cover. AP-193 correctly scopes the 0x34 row as ACE-sourced and unverifiable, and correctly notes the 2013 bounds check (cmp eax, 0x33 / ja) is evidence about the 2013 build, not the shipping one — matching §8.1 U4. AP-194 correctly states the GetDefaultOptionValue vs constructor-default disagreement for 0x2D/0x2F/0x32, correctly attributes it to the table's 0x2A bound, and correctly instructs future readers not to "fix" it — matching §8.2. The code reproduces the quirk rather than correcting it, which is the right call.

---

5. Verdict

APPROVE-WITH-FIXES.

The transcription work is excellent — all 53 rows are right, the golden vector is right and genuinely independent, the wire layout matches §2.3–§2.7 field for field, the flush state machine matches §3.3–§3.5 including the 480 s boundary, and unknown/0x35/0x36 ids cannot reach the wire from any seam. The two substantive gaps are that a declared deviation's stated cause is false (MUST-FIX 1) and that the retail function the shared seam claims to mirror "exactly" is only half ported, with no register row (SHOULD-FIX 2). Neither blocks the slice's data; both must be closed before OP4 puts a Character tab on top of this seam.