# 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 `_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 `u32`s 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.