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>
This commit is contained in:
Erik
parent
86c0a7e0ee
commit
0df0a60424
2 changed files with 924 additions and 0 deletions
477
docs/research/2026-08-10-op1-review-blast.md
Normal file
477
docs/research/2026-08-10-op1-review-blast.md
Normal file
|
|
@ -0,0 +1,477 @@
|
|||
# Campaign OP slice OP1 — regression / blast-radius review
|
||||
|
||||
**Commit under review:** `86c0a7e0` "feat(runtime,net): Campaign OP slice OP1 —
|
||||
full character-option table, dirty model, real 0x01A1 blob builder"
|
||||
**Reviewer lens:** what this change can break OUTSIDE its stated scope.
|
||||
Retail-mechanism fidelity (mask transcription, auto-save split, `PlayerModule::Pack`
|
||||
field order) is a sibling reviewer's lane and is only touched here where a
|
||||
mismatch would be a *regression* in already-working behavior.
|
||||
**Date:** 2026-08-10 · **Mode:** read-only (no build, no test run, no launch)
|
||||
|
||||
**Verdict: APPROVE-WITH-FIXES.**
|
||||
|
||||
---
|
||||
|
||||
## 0. Consumer enumeration (coverage record for the re-reviewer)
|
||||
|
||||
Both axes walked, per `feedback_blast_radius_single_lens.md`: (a) every caller of
|
||||
each changed surface, and (b) every *other* caller of each surface the change
|
||||
routes through.
|
||||
|
||||
### 0.1 `RuntimeCharacterOptionsState` — every consumer
|
||||
|
||||
| Consumer | Site | Member used | Changed by OP1? |
|
||||
|---|---|---|---|
|
||||
| `TurbineChatMembershipGate.Evaluate` | `src/AcDream.Runtime/Gameplay/TurbineChatMembershipGate.cs:108-129` | `Options1`, `Options2` | No — masks bit-identical (§1.1) |
|
||||
| `LiveSessionCommandRouter.RouteTurbineChat` | `src/AcDream.App/Net/LiveSessionCommandRouter.cs:346` | passes `CharacterState.Options` into the gate | No |
|
||||
| `LiveSessionEventRouter` (PlayerDescription) | `src/AcDream.Runtime/Session/LiveSessionEventRouter.cs:212` | `Replace(options1, options2)` | **Not updated** — see S5 |
|
||||
| `RuntimeCharacterState.CaptureOwnership` | `src/AcDream.Runtime/Gameplay/RuntimeCharacterState.cs:198, 211-213` | `Snapshot`, `DefaultOptions1/2` | **Ledger not widened** — see S3 |
|
||||
| `RuntimeCharacterState.View` (`IRuntimeCharacterView`) | `RuntimeCharacterState.cs:512` | `Snapshot` | No |
|
||||
| `GameRuntimeEvents` canonical checkpoint string | `src/AcDream.Runtime/GameRuntimeEvents.cs:177` | `Options.Revision` | Revision now *stops* advancing on a redundant toggle, and *starts* advancing for 47 previously-unmodeled ids — see N1 |
|
||||
| `InteractionRetainedUiComposition` | `src/AcDream.App/Composition/InteractionRetainedUiComposition.cs:318` | `Character.Options.DragItemOnPlayerOpensSecureTrade` | No |
|
||||
| `RuntimeCharacterState.ResetSession` / `Dispose` | `RuntimeCharacterState.cs:431, 457` | `Options.ResetSession()` | Now also clears `_isDirty` — correct (§3) |
|
||||
| `RuntimeGenerationReset` (J8 reset transaction) | `src/AcDream.Runtime/RuntimeGenerationReset.cs:272` | `_character.ResetSession` | Reaches the new dirty clear — correct (§3) |
|
||||
| `GameRuntime` construction | `src/AcDream.Runtime/GameRuntime.cs:199-200` | now passes `dependencies.TimeProvider` | New; no other construction site |
|
||||
| `LiveSessionRuntimeFactory.SendSingleCharacterOption` | `src/AcDream.App/Net/LiveSessionRuntimeFactory.cs:347-352` | `SetOptionBit` → **`TrySetOption`** | Rewritten |
|
||||
| `LiveSessionRuntimeFactory.SaveCharacterOptionsIfDirty` | `LiveSessionRuntimeFactory.cs:357-370` | `TryFlush` | New |
|
||||
| `DirectGameRuntimeCommandAdapter.SetSingleOption` | `src/AcDream.Runtime/Session/DirectGameRuntimeCommandAdapter.cs:666-670` | **`TrySetOption`** (was: wire send only) | Rewritten |
|
||||
| `DirectGameRuntimeCommandAdapter.SaveOptions` | `DirectGameRuntimeCommandAdapter.cs:684-697` | `TryFlush` | New |
|
||||
| `CharacterOptionsBlobSource.Capture` | `src/AcDream.Runtime/Gameplay/CharacterOptionsBlobSource.cs:41-47` | `Options1`, `Options2` | New |
|
||||
|
||||
No production caller of `SetOptionBit` survives outside `TrySetOption`
|
||||
(verified by grep). The only remaining direct callers are tests
|
||||
(`TurbineChatMembershipGateTests.cs:228/247`,
|
||||
`RuntimeCharacterStateTests.cs:249-282`,
|
||||
`LiveSessionCommandRouterTests.cs:497`).
|
||||
|
||||
### 0.2 `CharacterOptionId` (6 → 53 members) — every consumer
|
||||
|
||||
| Consumer | Site | Effect of the widening |
|
||||
|---|---|---|
|
||||
| `RuntimeSettingsController.SaveChat` | `src/AcDream.App/Settings/RuntimeSettingsController.cs:540-552` | none — still names the 5 Options2 chat ids by member |
|
||||
| `RetailClientCommandCatalog.JoinLeaveTags` | `src/AcDream.UI.Abstractions/Panels/Chat/RetailClientCommandCatalog.cs:302-312` | none — hard-coded raw `0x1B/0x23/0x24/0x25/0x26/0x2E` uints with comments; values re-verified against the widened enum, all six match |
|
||||
| `CharacterOptionTable` | `src/AcDream.Runtime/Gameplay/CharacterOptionTable.cs` | new — keys the table |
|
||||
| Tests (`RuntimeCharacterStateTests`, `RuntimeSettingsControllerTests`, `LiveSessionCommandRouterTests`, `DirectGameRuntimeCommandAdapterTests`, `TurbineChatMembershipGateTests`) | various | no exhaustiveness assumption anywhere |
|
||||
|
||||
No `switch` over `CharacterOptionId`, no `Enum.GetValues<CharacterOptionId>()`
|
||||
in production, no name-based serialization. The widening is safe — the old
|
||||
6-case `switch` in `SetOptionBit` was the only exhaustiveness-shaped consumer
|
||||
and it is gone.
|
||||
|
||||
### 0.3 `PlayerDescriptionParser.CharacterOptions1/2` — every consumer
|
||||
|
||||
| Consumer | Site | Touched? |
|
||||
|---|---|---|
|
||||
| `TurbineChatMembershipGate` | `TurbineChatMembershipGate.cs:109-129` | no |
|
||||
| `RuntimeSettingsController.SyncChatFromServerOptions` | `RuntimeSettingsController.cs:591-599` | no |
|
||||
| `RuntimeCharacterOptionsState.DefaultOptions1` | `RuntimeCharacterState.cs:635` | no |
|
||||
| `RuntimeCharacterOptionsSnapshot.DragItemOnPlayerOpensSecureTrade` | `RuntimeCharacterState.cs:618-621` | no |
|
||||
| `ChatSettings` doc | `src/AcDream.UI.Abstractions/Panels/Settings/ChatSettings.cs:20, 91` | no |
|
||||
|
||||
This enum is now a **second, independent** definition of the same bits that
|
||||
`CharacterOptionTable` defines — see S6.
|
||||
|
||||
### 0.4 `IRuntimeCharacterCommands` implementers (interface gained `SaveOptions`)
|
||||
|
||||
- `src/AcDream.Runtime/Session/DirectGameRuntimeCommandAdapter.cs:29` — updated
|
||||
- `src/AcDream.App/Runtime/CurrentGameRuntimeCommandAdapter.cs:28` — updated
|
||||
- `tests/AcDream.App.Tests/Composition/InteractionUiRuntimeSourcesTests.cs:219` — fake, updated
|
||||
|
||||
No plugin-facing implementer (`AcDream.Plugin.Abstractions` is BCL-only and does
|
||||
not see `AcDream.Runtime`). `AcDream.Headless` has no character-option command
|
||||
surface at all (grep over `src/AcDream.Headless` finds only config-file
|
||||
`session.Character.*`), so bots reach these verbs through
|
||||
`GameRuntime.Commands.Character` directly.
|
||||
|
||||
### 0.5 Routes that previously reached the deleted `SendSingleCharacterOption` closure
|
||||
|
||||
| Route | Chain | Post-OP1 behavior |
|
||||
|---|---|---|
|
||||
| Settings Chat toggles | `RuntimeSettingsController.PublishHearOptionChange` (`:561-566`) → `RuntimeSettingsTargets.SetSingleCharacterOption` (`:321-322`) → `SetSingleCharacterOptionRuntimeCmd` → `LiveSessionCommandRouter` (`:173-177`) → `bindings.SendSingleCharacterOption` → `TrySetOption` | one local write, then one `0x0005` — **unchanged** for all five ids (all `IsAutoSave`), except the new unchanged-value no-op (N1) |
|
||||
| `@join` / `@leave` | `ClientCommandController` (`:285-292`) → `Bindings.SetSingleCharacterOption` (wired at `LiveSessionRuntimeFactory.cs:491`) → same closure → `TrySetOption` | same — all six join/leave ids are `IsAutoSave = true` |
|
||||
| Router's guarded client-command projection | `LiveSessionCommandRouter.BuildGuardedClientCommands` (`:485-486`) → `InvokeClient` → same closure | same |
|
||||
| Graphical Runtime command | `CurrentGameRuntimeCommandAdapter.SetSingleOption` (`:668-695`) → `_commands.Publish` → router → same closure | same, plus a new pre-publish unknown-id rejection |
|
||||
| Headless / bot | `DirectGameRuntimeCommandAdapter.SetSingleOption` (`:652-675`) | **fixed** — previously sent the wire message with no local write |
|
||||
|
||||
`LiveCommandBus.Publish` (`src/AcDream.UI.Abstractions/LiveCommandBus.cs:48-63`)
|
||||
is **synchronous** dispatch, so there is no queue and no reordering window
|
||||
between a `SetSingleCharacterOptionRuntimeCmd` and a
|
||||
`SaveCharacterOptionsRuntimeCmd`; the local bit write completes inside the
|
||||
adapter call. **No double-write and no dropped write on any route** — each route
|
||||
performs exactly one `TrySetOption` and `TrySetOption` performs exactly one
|
||||
`SetOptionBit`.
|
||||
|
||||
---
|
||||
|
||||
## 1. Prompt item 1 — is the CH3/CH4 behavior bit-identical?
|
||||
|
||||
### 1.1 Mask equivalence (verified)
|
||||
|
||||
The six ids the old 6-case `switch` modeled resolve to **exactly** the same
|
||||
(word, mask) pairs in `CharacterOptionTable`:
|
||||
|
||||
| id | old switch (`RuntimeCharacterState.cs`, pre-OP1) | `CharacterOptionTable.cs` | match |
|
||||
|---|---|---|---|
|
||||
| `0x1B` ListenToAllegianceChat | Options1 `CharacterOptions1.HearAllegianceChat = 0x40000000` (`PlayerDescriptionParser.cs:215`) | `:138` Options1 `0x40000000` | yes |
|
||||
| `0x23` ListenToGeneralChat | Options2 `0x00000100` (`:232`) | `:146` Options2 `0x00000100` | yes |
|
||||
| `0x24` ListenToTradeChat | Options2 `0x00000200` (`:233`) | `:147` `0x00000200` | yes |
|
||||
| `0x25` ListenToLFGChat | Options2 `0x00000400` (`:234`) | `:148` `0x00000400` | yes |
|
||||
| `0x26` ListenToRoleplayChat | Options2 `0x00000800` (`:235`) | `:149` `0x00000800` | yes |
|
||||
| `0x2E` ListenToSocietyChat | Options2 `0x00080000` (`:236`) | `:157` `0x00080000` | yes |
|
||||
|
||||
All six carry `IsAutoSave = true`, so all six still take the immediate-`0x0005`
|
||||
branch of `TrySetOption` (`RuntimeCharacterState.cs:724-725`). The
|
||||
`TurbineChatMembershipGate` reads the same bits it always did. **CH3's gate and
|
||||
CH4's `@join`/`@leave` are behaviorally identical on every changed-value
|
||||
toggle.**
|
||||
|
||||
I also hand-verified all 53 `(IsOptions1, Mask)` pairs are pairwise distinct, and
|
||||
that OR-ing the `ClientDefault = true` rows reproduces `0x50C4A54A` /
|
||||
`0x00008700` (the table's own test at `CharacterOptionTableTests.cs:120-140`
|
||||
asserts this, and it holds independently).
|
||||
|
||||
### 1.2 The one real behavior change (N1)
|
||||
|
||||
`TrySetOption` early-returns when the bit already holds the requested value
|
||||
(`RuntimeCharacterState.cs:718-720`). The pre-OP1 closure wrote and sent
|
||||
unconditionally. So:
|
||||
|
||||
- `/join general` when already listening now sends **nothing** (was: a redundant
|
||||
`0x0005`).
|
||||
- A Settings save whose persisted `ChatSettings` diff says "changed" but whose
|
||||
Runtime bit already matches now sends **nothing**.
|
||||
|
||||
I verified against retail: `PlayerModule::SetHearGeneralChat @0x005D35C0`
|
||||
(`docs/research/named-retail/acclient_2013_pseudo_c.txt:486939`) opens with
|
||||
`if ((((int8_t)(this->options2_ >> 8)) & 1) == arg2) return;` — the early return
|
||||
is real, and `ClientCommunicationSystem::DoJoinChat @0x0056F510` emits no
|
||||
confirmation text of its own, so retail's repeat `/join` is also silent. **This
|
||||
change is retail-faithful.** It is listed as a NOTE, not a defect, because it
|
||||
removes acdream's only *manual* resync path if the local bit and ACE's stored
|
||||
option ever diverge (e.g. a `0x0005` that ACE dropped pre-`LoginComplete`) —
|
||||
retail has the identical limitation, so this is a fidelity-consistent
|
||||
regression in robustness only.
|
||||
|
||||
---
|
||||
|
||||
## 2. Prompt item 2 — the deleted closure and the two adapters
|
||||
|
||||
Traced above (§0.5). Summary: **exactly one local write and at most one send on
|
||||
every route; no double-apply, no dropped write, no reordering.**
|
||||
|
||||
On the specific "DIRECT adapter double-apply when a PlayerDescription re-seed
|
||||
races a toggle": `TrySetOption` reads the word (`:718`), compares, then calls
|
||||
`SetOptionBit` which **re-reads** the word fresh (`:754/:759`) before the
|
||||
read-modify-write. A `Replace` landing between the two reads therefore cannot
|
||||
produce a double-apply — the OR/AND-NOT is applied once, to whichever word is
|
||||
current. The opposite interleaving (`SetOptionBit` writes, then `Replace`
|
||||
overwrites) *loses* the local write; that is a pre-existing shape inherited from
|
||||
CH4 for auto-save ids, but OP1 extends it to batched ids where it now has a new
|
||||
consequence — see S5.
|
||||
|
||||
---
|
||||
|
||||
## 3. Prompt item 3 — dirty model, reset, cross-character contamination
|
||||
|
||||
**Clean.** `RuntimeCharacterOptionsState.ResetSession` clears `_isDirty`
|
||||
(`RuntimeCharacterState.cs:826-827`), and it is reached from both owners of
|
||||
character lifetime:
|
||||
|
||||
- generation replacement / reconnect: `RuntimeGenerationReset.cs:272`
|
||||
`Advance(state, _character.ResetSession)` → `RuntimeCharacterState.ResetSession`
|
||||
(`:425-443`) → `Try(Options.ResetSession, ...)` (`:431`)
|
||||
- terminal disposal: `RuntimeCharacterState.Dispose` (`:457`)
|
||||
|
||||
So a dirty flag from character A cannot survive into character B's generation,
|
||||
and cannot cause a `SaveOptions` blob to be sent with B's identity carrying A's
|
||||
intent. `_firstDirtiedAt` is deliberately left stale but is unreachable while
|
||||
`_isDirty == false` (`FirstDirtiedAt` getter, `:676-679`) and is overwritten by
|
||||
the next `MarkDirty`. Pinned by
|
||||
`RuntimeCharacterStateTests.ResetSession_ClearsDirtyState` (`:427-439`).
|
||||
|
||||
Two gaps around this, both S-grade: S3 (the ownership ledger does not observe
|
||||
the new field) and S5 (`Replace` does not).
|
||||
|
||||
---
|
||||
|
||||
## 4. Prompt item 4 — is the blob echo stale?
|
||||
|
||||
**No. The echo reads the canonical live owners, not a parser-side copy.**
|
||||
Verified source by source:
|
||||
|
||||
| Echo field | Read from | Canonical? |
|
||||
|---|---|---|
|
||||
| `Options1/Options2` | `character.Options` (`CharacterOptionsBlobSource.cs:42-43`) | yes — the same object `TrySetOption` writes and the gate reads |
|
||||
| `Shortcuts` | `shortcuts.Items` — `RuntimeInventoryState.Shortcuts` (`RuntimeInventoryState.cs:81`), the J4.5 `ShortcutStore` | yes — `CurrentGameRuntimeCommandAdapter.AddShortcut` (`:426-428`) goes through `RuntimeInventoryState.TryAddShortcut` (`:119-140`) which writes `Shortcuts.Set(entry)`; `RemoveShortcut` likewise (`:161`) |
|
||||
| `FavoriteSpells` | `character.Spellbook.GetFavorites(tab)` × 8 (`CharacterOptionsBlobSource.cs:38-39`) | yes — `CurrentGameRuntimeCommandAdapter.AddFavorite/RemoveFavorite` (`:477, :508`) mutate the same `Spellbook` |
|
||||
| `DesiredComponents` | `character.Spellbook.DesiredComponents` (`Spellbook.cs:219`) | yes — same J4.3 spellbook the UI mutates |
|
||||
| `SpellbookFilters` | `character.Spellbook.SpellbookFilters` (`Spellbook.cs:221`) | yes — `SpellbookWindowController.cs:254` writes via `SetSpellbookFilters` |
|
||||
|
||||
`ShortcutStore.Items` returns a `Volatile.Read` of an immutable array
|
||||
(`ShortcutStore.cs:37`) and `GetFavorites` returns `.ToArray()`
|
||||
(`Spellbook.cs:216`), so those two are snapshot-safe. `DesiredComponents`
|
||||
returns the **live** dictionary — see N4.
|
||||
|
||||
**There is, however, an unrelated wipe hazard on the same verb: see M1.**
|
||||
|
||||
---
|
||||
|
||||
## 5. Prompt item 5 — wire safety
|
||||
|
||||
Both adapters gate on generation + `IsInWorld`:
|
||||
`CurrentGameRuntimeCommandAdapter.Validate(expectedGeneration, requireWorld: true)`
|
||||
(`:791-820`) and `DirectGameRuntimeCommandAdapter.Validate` (`:1111-1132`, which
|
||||
additionally requires a non-null route and session). The graphical route then
|
||||
passes `LiveSessionCommandRouter.SendIfActive` (`:523-532`), which is `false`
|
||||
before `Activate()` and after `Dispose()`. **Post-teardown sends are impossible
|
||||
on both hosts.**
|
||||
|
||||
Pre-`LoginComplete` is a different story. `LiveSessionController` sets
|
||||
`_inWorld = true` at `:587`, immediately after `_operations.EnterWorld(...)`
|
||||
returns at `:580` — i.e. as soon as the `CharacterEnterWorld` body is *sent*.
|
||||
`GameActionLoginComplete` is only emitted later, from the inbound stream, when
|
||||
the local player's own `CreateObject` is accepted
|
||||
(`RuntimeLiveEntitySessionController.cs:176-183` for the content-less host,
|
||||
`:858` for the graphical placement conductor). So `IsInWorld == true` does **not**
|
||||
imply `LoginComplete` has been sent, and there is a real window in which a
|
||||
`0x01A1` would be silently dropped by ACE (lane C §5). This is the same window
|
||||
`0x0005` has had since CH3, so OP1 does not newly create it — but it does newly
|
||||
route a *destructive* message through it. See **M1**.
|
||||
|
||||
There is currently **no production caller of `SaveOptions`** — grep finds only
|
||||
the two adapters, the router registration, and tests. `MarkDirty` is likewise
|
||||
unreachable in production today: the only production entrances
|
||||
(`@join`/`@leave` via `JoinLeaveTags`, and the five Settings Chat toggles) all
|
||||
carry auto-save ids. So today's *live* blast radius of the blob path is
|
||||
effectively zero; M1/S2 are pre-emptive against the very next slice.
|
||||
|
||||
---
|
||||
|
||||
## 6. Findings
|
||||
|
||||
### MUST-FIX
|
||||
|
||||
**M1 — `SaveOptions` has no "server state seeded yet" gate; firing it before the
|
||||
first `PlayerDescription` overwrites the character's server-side options with
|
||||
client defaults.**
|
||||
`RuntimeCharacterOptionsState` starts at the *client* constructor defaults
|
||||
`DefaultOptions1 = 0x50C4A54A` / `DefaultOptions2 = 0x00948700`
|
||||
(`src/AcDream.Runtime/Gameplay/RuntimeCharacterState.cs:634-636`) and is only
|
||||
corrected to server truth when `Replace` runs from
|
||||
`LiveSessionEventRouter.cs:212`. The command gate is `IsInWorld`
|
||||
(`DirectGameRuntimeCommandAdapter.cs:1128`,
|
||||
`CurrentGameRuntimeCommandAdapter.cs:800`), which as shown in §5 goes true
|
||||
before the description arrives. A bot (or, after TS-71, a logout hook) that does
|
||||
"toggle a batched option, then `SaveOptions`" inside that window sends a
|
||||
`0x01A1` whose `options1`/`options2` — the two fields ACE actually *stores* —
|
||||
are the client defaults, not the character's. `spellbookFilters` is also always
|
||||
packed (header bit `0x020` is unconditional,
|
||||
`src/AcDream.Core.Net/Messages/SocialActions.cs:74-77`) and would be `0`.
|
||||
That is exactly the "wipe server-side state" failure class this review was asked
|
||||
to treat as MUST-FIX; it is simply reached through the *options* fields rather
|
||||
than the echo fields. Reachability is narrow today (no production caller, and
|
||||
`MarkDirty` is unreachable from any wired entrance), which is why the verdict is
|
||||
APPROVE-WITH-FIXES rather than REJECT — but the guard must land before TS-71
|
||||
wires the logout/timer triggers or before any Options-panel Apply button exists.
|
||||
Suggested fix: a `HasServerSeed` latch set by `Replace` and required by
|
||||
`TryFlush`/`TryFlushIfAutoSaveDue` (cleared by `ResetSession`), with a test.
|
||||
|
||||
### SHOULD-FIX
|
||||
|
||||
**S1 — the CH3/CH4 regression test no longer exercises the production closure; it
|
||||
now mirrors the *pre-OP1* shape.**
|
||||
`tests/AcDream.App.Tests/Net/LiveSessionCommandRouterTests.cs:493-499` substitutes
|
||||
its own binding — `characterState.Options.SetOptionBit(id, value); sent.Add(...)`
|
||||
— and its comment still says "Mirrors LiveSessionRuntimeFactory.
|
||||
CreateCommandBindings' shared SendSingleCharacterOption local function". After
|
||||
OP1 that function is `TrySetOption` with an unchanged-value early return
|
||||
(`LiveSessionRuntimeFactory.cs:347-352`), so the mirror has silently drifted
|
||||
from the thing it is supposed to protect. No test anywhere drives the real
|
||||
closure. This is precisely the "tests that pin a wrong shape and look green"
|
||||
failure the commit message cites as the reason the CH3 `0x01A1` builder died.
|
||||
Fix: make the test construct the real binding, or extract the closure so it can
|
||||
be exercised directly.
|
||||
|
||||
**S2 — `TryFlush` / `TryFlushIfAutoSaveDue` invoke a caller-supplied callback
|
||||
while holding `_dirtyGate`, which sets up a lock inversion that TS-71's own
|
||||
follow-up will trigger.**
|
||||
`RuntimeCharacterState.cs:766-777` and `:786-796` call `flush()` inside
|
||||
`lock (_dirtyGate)`. Today every flush originates *inside*
|
||||
`LiveSessionCommandRouter.SendIfActive`, which already holds `_gate`
|
||||
(`LiveSessionCommandRouter.cs:523-532`), so the order is consistently
|
||||
`_gate → _dirtyGate`. The natural TS-71 wiring — a per-frame
|
||||
`Options.TryFlushIfAutoSaveDue(() => _commands.Publish(new SaveCharacterOptionsRuntimeCmd()))`
|
||||
on the update thread — inverts it: that thread would hold `_dirtyGate` and then
|
||||
want `_gate`, while the render thread holds `_gate` (routing a
|
||||
`SetSingleCharacterOptionRuntimeCmd`) and wants `_dirtyGate` via `MarkDirty`.
|
||||
Deadlock. It also means a UDP send happens under the lock, blocking the
|
||||
`IsDirty`/`FirstDirtiedAt` getters. Fix: decide and clear under the lock, invoke
|
||||
the callback outside it (and document what happens if the send throws — today's
|
||||
`flush(); _isDirty = false;` ordering correctly *keeps* the module dirty on a
|
||||
throw, which is the behavior to preserve).
|
||||
|
||||
**S3 — the ownership ledger does not observe the new dirty state.**
|
||||
`RuntimeCharacterOwnershipSnapshot.IsConverged`
|
||||
(`RuntimeCharacterState.cs:26-40`) checks `OptionsAreDefaults` but has no
|
||||
`OptionsAreClean`. The project's J3.6/J8 discipline is "the combined ownership
|
||||
ledger converges to zero"; a new piece of session state that reset must clear is
|
||||
exactly what that ledger exists to catch. `CaptureOwnership` (`:167-225`) should
|
||||
carry `IsDirty == false`.
|
||||
|
||||
**S4 — `IRuntimeCharacterCommands.SaveOptions` has host-dependent result
|
||||
semantics, and encodes a boolean in a typed object-id field.**
|
||||
`DirectGameRuntimeCommandAdapter.SaveOptions` returns
|
||||
`primaryObjectId: flushed ? 1u : 0u` (`:701`); `CurrentGameRuntimeCommandAdapter.
|
||||
SaveOptions` always returns `0` (`:704-707`) because its flush is deferred onto
|
||||
the router. A consumer of the shared interface therefore gets different answers
|
||||
from the two hosts. Worse, `PrimaryObjectId` flows straight into the K2 bot-facing
|
||||
ordered event stream (`GameRuntimeEventHub.cs:151-161`,
|
||||
`GameRuntimeEvents.cs:36/144/242`), where `1` reads as object guid `0x00000001`.
|
||||
Either add a dedicated result field or drop the encoding and make both hosts
|
||||
return `0`.
|
||||
|
||||
**S5 — a PlayerDescription re-seed discards a pending batched change but leaves
|
||||
the module dirty.**
|
||||
`RuntimeCharacterOptionsState.Replace` (`:684-689`) overwrites both words
|
||||
wholesale and does not touch `_isDirty`. If the user flips a batched option and a
|
||||
fresh `PlayerDescription` lands before the flush, the local intent is lost while
|
||||
`IsDirty` stays `true`; the eventual `SaveOptions` then echoes the server's own
|
||||
words back at it. Harmless on the wire, silently wrong for the user. No test
|
||||
covers `Replace` against a dirty module. At minimum this needs a decision
|
||||
recorded (retail's `PlayerModule` is likewise clobbered by a re-seed, so
|
||||
"reproduce it" may be the right answer) plus a test that pins whichever
|
||||
behavior is chosen.
|
||||
|
||||
**S6 — the same bits are now defined twice, with nothing asserting they agree.**
|
||||
`PlayerDescriptionParser.CharacterOptions1/2`
|
||||
(`src/AcDream.Core.Net/Messages/PlayerDescriptionParser.cs:206-237`) is the
|
||||
*read* path — `TurbineChatMembershipGate.cs:109-129` and
|
||||
`RuntimeSettingsController.cs:591-599` both key off it. `CharacterOptionTable` is
|
||||
now the *write* path. I verified by hand that all six overlapping bits agree
|
||||
today, but the two definitions are in different projects with no cross-check.
|
||||
Edit one without the other and the write path and the membership gate diverge
|
||||
silently — the exact CH3 failure mode. Add a conformance test asserting
|
||||
`CharacterOptionTable`'s six chat rows equal the `PlayerDescriptionParser` enum
|
||||
values (or have one derive from the other).
|
||||
|
||||
### NOTE
|
||||
|
||||
**N1 — observable behavior deltas from the unchanged-value early return.**
|
||||
Repeat `/join`/`/leave` and a redundant Settings save now emit no `0x0005`
|
||||
(retail-faithful, verified against `SetHearGeneralChat @0x005D35C0` and
|
||||
`DoJoinChat @0x0056F510`). Two second-order effects worth knowing: the manual
|
||||
resync path for a client/server option desync is gone, and
|
||||
`Options.Revision` — which is embedded in the canonical checkpoint string at
|
||||
`GameRuntimeEvents.cs:177` used by the connected lifecycle/reconnect gates — no
|
||||
longer advances on a redundant toggle, while it *now can* advance for 47
|
||||
previously-unmodeled ids. Any recorded checkpoint baseline that captured a
|
||||
redundant toggle would drift.
|
||||
|
||||
**N2 — cross-project doc-comment `cref`s in `SocialActions.cs`.**
|
||||
`src/AcDream.Core.Net/Messages/SocialActions.cs:348` writes
|
||||
`<see cref="AcDream.Runtime.Gameplay.CharacterOptionTable"/>` from
|
||||
`AcDream.Core.Net`, which does not (and must not) reference `AcDream.Runtime`.
|
||||
`:201` uses `<see cref="favoriteSpells"/>` for a parameter (should be
|
||||
`paramref`). Both are inert today because no project sets
|
||||
`GenerateDocumentationFile`, but every project sets
|
||||
`TreatWarningsAsErrors` — enabling XML docs later would turn these into build
|
||||
breaks. The first is also an upward layering reference in documentation.
|
||||
|
||||
**N3 — `CharacterOptionTable`'s static state is outside the contract guard.**
|
||||
`GameRuntimeContractTests.J4GameplayOwnersHaveNoStaticMutableSessionState`
|
||||
(`tests/AcDream.Runtime.Tests/GameRuntimeContractTests.cs:218-246`) enumerates an
|
||||
explicit type list that does not include `CharacterOptionTable`, whose
|
||||
`private static readonly Dictionary` (`:83`) and `All` property (`:93-94`) are
|
||||
process-wide shared state. Immutable in practice, and the commit correctly made
|
||||
`AutoSaveDelay` a property to satisfy that guard on
|
||||
`RuntimeCharacterOptionsState` — but the table itself is unguarded. Also, `All`'s
|
||||
initializer reads `Entries`, so the two declarations are order-coupled (`:83`
|
||||
before `:93`); correct today, fragile under reordering.
|
||||
|
||||
**N4 — `BuildSetCharacterOptions` sizes and then enumerates a live dictionary.**
|
||||
`SocialActions.cs:232` and `:255-256` read `desiredComponents.Count` for the
|
||||
buffer size and then `foreach` the same collection, and
|
||||
`CharacterOptionsBlobSource.Capture` hands it
|
||||
`Spellbook.DesiredComponents`, which returns the **live** `_desiredComponents`
|
||||
reference (`src/AcDream.Core/Spells/Spellbook.cs:219`), not a copy. A mutation
|
||||
between sizing and writing throws (span slice) or mis-sizes the body. Same-thread
|
||||
under the #368 single-update-thread contract, so not reachable today; copying in
|
||||
`Capture` would close it for a couple of allocations.
|
||||
|
||||
**N5 — `LockUI` now has two homes.**
|
||||
`CharacterOptionId.LockUI` (`0x33`) is modeled as an auto-save option in the
|
||||
table (`CharacterOptionTable.cs:162`), while the Settings "Lock UI" toggle still
|
||||
routes to the purely local `RuntimeSettingsTargets.ApplyUiLock`
|
||||
(`src/AcDream.App/Settings/RuntimeSettingsTargets.cs:305`, reached from
|
||||
`RuntimeSettingsController.cs:349/509`). Nothing wrong today; it becomes a
|
||||
two-sources-of-truth bug the moment the Options panel binds `0x33`.
|
||||
|
||||
**N6 — the echo's canonicality is not pinned by a test.**
|
||||
`DirectGameRuntimeCommandAdapterTests.SaveOptions_FlushesTheDirtyBlobThenNoOpsWhenClean`
|
||||
(`tests/AcDream.Runtime.Tests/Session/DirectGameRuntimeCommandAdapterTests.cs:347-375`)
|
||||
asserts only the opcode at offset 8. Nothing asserts that a shortcut added
|
||||
in-session appears in the flushed blob. A regression from
|
||||
`CharacterOptionsBlobSource.Capture`'s canonical reads to a parser-side copy —
|
||||
the failure the prompt calls out — would pass every current test.
|
||||
|
||||
**N7 — no uniqueness test on the table.**
|
||||
`CharacterOptionTableTests` pins count, contiguity, the auto-save split, the
|
||||
client-default split, and the reconstructed default words, but not "no two
|
||||
entries share the same `(IsOptions1, Mask)`". The reconstruction test cannot
|
||||
catch a duplicate because `|=` is idempotent. I verified uniqueness by hand
|
||||
across all 53 rows; a one-line test would make it permanent.
|
||||
|
||||
**N8 — the golden-vector test's round-trip couples an outbound builder to an
|
||||
inbound parser.**
|
||||
`SocialActionsTests.BuildSetCharacterOptions_RoundTripsThroughPlayerDescriptionParser`
|
||||
(`tests/AcDream.Core.Net.Tests/Messages/SocialActionsTests.cs:242-296`) validates
|
||||
the body against acdream's own `PlayerDescriptionParser`. Where the parser is
|
||||
lenient — notably the `PackableHashTable` size word, which
|
||||
`SocialActions.cs:251-253` deliberately writes as a bare count while retail
|
||||
writes `(tableSize << 16) | count` — a lenient parser makes the round trip green
|
||||
regardless. The hand-computed golden vector (`:170-198`) is the load-bearing
|
||||
test; the round trip is corroboration only. (Whether the bare count is the right
|
||||
wire value is the fidelity reviewer's call.)
|
||||
|
||||
---
|
||||
|
||||
## 7. What I checked and found clean
|
||||
|
||||
- No production caller of `SetOptionBit` outside `TrySetOption`.
|
||||
- No `switch`/`Enum.GetValues` exhaustiveness dependency on `CharacterOptionId`.
|
||||
- `RetailClientCommandCatalog.JoinLeaveTags`' six hard-coded ids all still map to
|
||||
auto-save rows, so `@join`/`@leave` still sends `0x0005`.
|
||||
- `LiveCommandBus.Publish` is synchronous — no reorder window between a toggle
|
||||
and a flush.
|
||||
- `SaveCharacterOptionsRuntimeCmd` is generation/activity gated identically to
|
||||
every other router command (`SendIfActive`), pinned by
|
||||
`LiveSessionCommandRouterTests.cs:444-456`.
|
||||
- `Character` domain operation ids: `Advance` uses `(int)RuntimeAdvancementKind`
|
||||
= 0–3 (`GameRuntimeCommands.cs:222-228`), options use 4 and 5 — no collision on
|
||||
either adapter.
|
||||
- Both adapters were updated for the new interface member; the only other
|
||||
implementer is a test fake.
|
||||
- The J8 generation-reset transaction reaches the new dirty clear.
|
||||
- `TryFlush` leaves the module dirty if the send throws (correct direction).
|
||||
- `SaveCharacterOptionsIfDirty` captures the per-session `WorldSession` passed
|
||||
into `CreateCommandBindings`, so no stale-session capture across generations.
|
||||
- All 53 table masks are pairwise distinct; `ClientDefault` reconstruction
|
||||
reproduces `0x50C4A54A` / `0x00008700`; the constructor default `0x00948700`
|
||||
decomposes exactly as the register row claims.
|
||||
- Register rows for TS-71, AP-193 and AP-194 are present in the same commit.
|
||||
|
||||
---
|
||||
|
||||
## 8. Verdict
|
||||
|
||||
**APPROVE-WITH-FIXES.** Nothing in the diff regresses behavior that works
|
||||
today: the six chat masks are bit-identical, every route still performs exactly
|
||||
one local write followed by at most one send, the J8 reset clears the new dirty
|
||||
state, and the blob echo reads the canonical live owners rather than a
|
||||
parser-side copy. The headless local-write gap is a genuine fix. The blocking
|
||||
item (M1) is a missing readiness gate on a newly-introduced *destructive* wire
|
||||
verb that is not yet production-called — it must land before TS-71 wires the
|
||||
logout/timer triggers. S1 (a regression test that now mirrors the old shape),
|
||||
S2 (a lock-inversion landmine aimed squarely at TS-71), and S6 (duplicated bit
|
||||
definitions across projects with no cross-check) are the other items I would
|
||||
want closed inside this campaign rather than deferred.
|
||||
447
docs/research/2026-08-10-op1-review-mechanism.md
Normal file
447
docs/research/2026-08-10-op1-review-mechanism.md
Normal file
|
|
@ -0,0 +1,447 @@
|
|||
# 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 `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.
|
||||
Loading…
Add table
Add a link
Reference in a new issue