The 26-name allow-list cross-checks clean against research §5.2, CharacterOptionTable, and retail's PlayerOption enum — all three risky renames verified right (ToggleRun IS PlayerOption 0x0A run-as-default, not the keybind latch). #368 honored; existing configs byte-identical; per-route seeder leak-free; the Runtime observation hook never fires in the graphical host. MUST-FIX 1: HeadlessProcessHost.WithAccount hand-copies six of seven descriptor fields — CharacterOptions is dropped, so the K3 direct-CLI launch mode silently no-ops the whole feature. MUST-FIX 2: a truncated PlayerDescription RE-seed re-opens OP1's wipe class — Replace installs zeroed words while the latch stays armed from the earlier complete seed, and OP7's automation then flushes those zeros into 0x01A1; fix at the seam (a truncated parse installs nothing, notifies no one). SHOULD: validate schema names as strings (Enum.TryParse accepts numeric and comma-combined keys that alias into the allow-list); reject the contradictory fellowship pair at load (declared both-true oscillates against retail's mutual exclusion); move the mid-teleport hook to the sequence tail; cover the three production LoginComplete hooks with the real controller argument; correct the OP7 test-script line expecting a 0x0005 for batched SalvageMultiple (only the 0x01A1 carries it). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
412 lines
26 KiB
Markdown
412 lines
26 KiB
Markdown
# Campaign OP slice OP7 — combined dual-lens review
|
|
|
|
**Commit under review:** `09cb548a` ("feat(headless): Campaign OP slice OP7 —
|
|
declared characterOptions with seed-diff sends")
|
|
**Contract:** `docs/plans/2026-08-10-options-panel-campaign.md` §4 OP7 + §2 D8
|
|
**Research base:** `docs/research/2026-08-10-character-options-map.md` §5.2/§7.3;
|
|
`docs/research/2026-08-10-set-character-options-wire.md` §3.5/§5
|
|
**Reviewer:** one agent carrying BOTH lenses (mechanism/contract + blast radius),
|
|
per the coordinator's smaller-slice protocol.
|
|
**Method:** read-only. No build, no test run, no client launch. All verdicts are
|
|
source-derived with `file:line` evidence.
|
|
|
|
## Verdict
|
|
|
|
**APPROVE-WITH-FIXES** — two MUST-FIX, five SHOULD-FIX, seven NOTEs.
|
|
|
|
The core mechanism is right: the 26-name allow-list is an exact, correct
|
|
transcription of research §5.2's tier-1+tier-2 rows (every one of the risky
|
|
prose→enum renames resolves to the correct `PlayerOption` id — see the table in
|
|
§1); the diff engine's send policy matches wire research §3.5 and §5.5
|
|
exactly; the two-precondition latch is genuinely per-connect correct (it is
|
|
paired with `RuntimeCharacterOptionsState.ResetSession`'s own
|
|
`HasServerSeed`/word reset at generation reset); and #368 is honoured —
|
|
the whole diff is synchronous delegate plumbing with zero `async`/`Task`/
|
|
continuation added anywhere under `src/`.
|
|
|
|
The two MUST-FIX findings are both *silent* failures rather than crashes: one
|
|
production launch mode drops the declared block entirely, and one degraded-parse
|
|
path lets the seeder act on words OP1's own R3 fix already declared untrusted.
|
|
|
|
---
|
|
|
|
## 1. The 26-name → id cross-check (mechanism lens, item 1)
|
|
|
|
Allow-list under review: `src/AcDream.Headless/Configuration/HeadlessConfigurationLoader.cs:26-57`.
|
|
Enum: `src/AcDream.Core.Net/Messages/SocialActions.cs:362-417`.
|
|
Table: `src/AcDream.Runtime/Gameplay/CharacterOptionTable.cs:111-165`.
|
|
Retail ground truth: `docs/research/named-retail/acclient.h:4165-4217`
|
|
(`enum PlayerOption`).
|
|
Research rows: `character-options-map.md` §5.2 (`:467-483`) with per-row
|
|
storage/wire columns from §2.1-§2.7.
|
|
|
|
Legend: **Word/Mask** from `CharacterOptionTable`; **Wire** S = auto-save
|
|
(`0x0005` immediately), B = batched (dirties the `0x01A1` module);
|
|
research §2's own Wire column is the cross-check.
|
|
|
|
### Tier 1 (22 rows)
|
|
|
|
| # | Research §5.2 prose name | id | Allow-list member | Enum value | retail `acclient.h` name | Word · Mask | Wire (table / §2) | Verdict |
|
|
|---|---|---|---|---|---|---|---|---|
|
|
| 1 | `IgnoreAllegianceRequests` | 0x01 | `IgnoreAllegianceRequests` | 0x01 | `IgnoreAllegianceRequests_PlayerOption` | O1 · `0x00000004` | S / S (§2.3) | ✅ |
|
|
| 2 | `IgnoreFellowshipRequests` | 0x02 | `IgnoreFellowshipRequests` | 0x02 | `IgnoreFellowshipRequests_PlayerOption` | O1 · `0x00000008` | S / S (§2.3) | ✅ |
|
|
| 3 | `IgnoreAllTradeRequests` | 0x03 | **`IgnoreTradeRequests`** (rename) | 0x03 | `IgnoreTradeRequests_PlayerOption` | O1 · `0x00020000` | B / B (§2.4) | ✅ |
|
|
| 4 | `LetOtherPlayersGiveYouItems` | 0x06 | **`AllowGive`** (rename) | 0x06 | `AllowGive_PlayerOption` | O1 · `0x00000040` | B / B (§2.4) | ✅ |
|
|
| 5 | `ShareFellowshipExpAndLuminance` | 0x0F | **`FellowshipShareXP`** (rename) | 0x0F | `FellowshipShareXP_PlayerOption` | O1 · `0x00040000` | S / S (§2.3) | ✅ |
|
|
| 6 | `AcceptCorpseLootingPermissions` | 0x10 | **`AcceptLootPermits`** (rename) | 0x10 | `AcceptLootPermits_PlayerOption` | O1 · `0x00080000` | S / S (§2.4) | ✅ |
|
|
| 7 | `ShareFellowshipLoot` | 0x11 | `FellowshipShareLoot` | 0x11 | `FellowshipShareLoot_PlayerOption` | O1 · `0x00100000` | S / S (§2.3) | ✅ |
|
|
| 8 | `AutomaticallyAcceptFellowshipRequests` | 0x12 | **`FellowshipAutoAcceptRequests`** (rename) | 0x12 | `FellowshipAutoAcceptRequests_PlayerOption` | O1 · `0x20000000` | S / S (§2.3) | ✅ |
|
|
| 9 | `ShowAllegianceLogons` | 0x18 | **`DisplayAllegianceLogonNotifications`** (rename) | 0x18 | `DisplayAllegianceLogonNotifications_PlayerOption` | O1 · `0x08000000` | B / B (§2.3) | ✅ |
|
|
| 10 | `UseChargeAttack` | 0x19 | `UseChargeAttack` | 0x19 | `UseChargeAttack_PlayerOption` | O1 · `0x10000000` | S / S (§2.5) | ✅ |
|
|
| 11 | `UseCraftingChanceOfSuccessDialog` | 0x1A | **`UseCraftSuccessDialog`** (rename) | 0x1A | `UseCraftSuccessDialog_PlayerOption` | O1 · `0x80000000` | B / B (§2.2) | ✅ |
|
|
| 12 | `AutoRepeatAttacks` | 0x00 | **`AutoRepeatAttack`** (rename) | 0x00 | `AutoRepeatAttack_PlayerOption` | O1 · `0x00000002` | S / S (§2.5) | ✅ |
|
|
| 13 | `LeadMissileTargets` | 0x2A | `LeadMissileTargets` | 0x2A | `LeadMissileTargets_PlayerOption` | O2 · `0x00008000` | S / S (§2.5) | ✅ |
|
|
| 14 | `UseFastMissiles` | 0x2B | `UseFastMissiles` | 0x2B | `UseFastMissiles_PlayerOption` | O2 · `0x00010000` | S / S (§2.5) | ✅ |
|
|
| 15 | `ConfirmUseOfRareGems` | 0x2D | **`ConfirmVolatileRareUse`** (rename) | 0x2D | `ConfirmVolatileRareUse_PlayerOption` | O2 · `0x00040000` | B / B (§2.2) | ✅ |
|
|
| 16 | `AppearOffline` | 0x27 | `AppearOffline` | 0x27 | `AppearOffline_PlayerOption` | O2 · `0x00001000` | S / S (§2.7) | ✅ |
|
|
| 17 | `ListenToAllegianceChat` | 0x1B | `ListenToAllegianceChat` | 0x1B | `HearAllegianceChat_PlayerOption` † | O1 · `0x40000000` | S / S (§2.6) | ✅ |
|
|
| 18 | `ListenToGeneralChat` | 0x23 | `ListenToGeneralChat` | 0x23 | `HearGeneralChat_PlayerOption` † | O2 · `0x00000100` | S / S (§2.6) | ✅ |
|
|
| 19 | `ListenToTradeChat` | 0x24 | `ListenToTradeChat` | 0x24 | `HearTradeChat_PlayerOption` † | O2 · `0x00000200` | S / S (§2.6) | ✅ |
|
|
| 20 | `ListenToLFGChat` | 0x25 | `ListenToLFGChat` | 0x25 | `HearLFGChat_PlayerOption` † | O2 · `0x00000400` | S / S (§2.6) | ✅ |
|
|
| 21 | `ListenToRoleplayChat` | 0x26 | `ListenToRoleplayChat` | 0x26 | `HearRoleplayChat_PlayerOption` † | O2 · `0x00000800` | S / S (§2.6) | ✅ |
|
|
| 22 | `ListenToSocietyChat` | 0x2E | `ListenToSocietyChat` | 0x2E | `HearSocietyChat_PlayerOption` † | O2 · `0x00080000` | S / S (§2.6) | ✅ |
|
|
|
|
† The six `ListenTo*Chat` members are the deliberate pre-existing CH3 spelling
|
|
divergence from retail's `Hear*Chat_PlayerOption`, documented at
|
|
`SocialActions.cs:354-357`. Ids are identical; the rename is ours and
|
|
established, not introduced here.
|
|
|
|
### Tier 2 (4 rows)
|
|
|
|
| # | Research §5.2 prose name | id | Allow-list member | Enum value | retail `acclient.h` name | Word · Mask | Wire (table / §2) | Verdict |
|
|
|---|---|---|---|---|---|---|---|---|
|
|
| 23 | `UseMainPackAsDefaultForPickingUpItems` | 0x29 | **`MainPackPreferred`** (rename) | 0x29 | `MainPackPreferred_PlayerOption` | O2 · `0x00004000` | B / B (§2.1) | ✅ |
|
|
| 24 | `RunAsDefaultMovement` | 0x0A | **`ToggleRun`** (rename) | 0x0A | `ToggleRun_PlayerOption` | O1 · `0x00000400` | B / B (§2.5) | ✅ |
|
|
| 25 | `AutoTarget` | 0x0D | `AutoTarget` | 0x0D | `AutoTarget_PlayerOption` | O1 · `0x00002000` | B / B (§2.5) | ✅ |
|
|
| 26 | `SalvageMultipleMaterialsAtOnce` | 0x22 | **`SalvageMultiple`** (rename) | 0x22 | `SalvageMultiple_PlayerOption` | O2 · `0x00000080` | B / B (§2.1) | ✅ |
|
|
|
|
**The three renames the review brief flagged as risky are all correct:**
|
|
|
|
* **`RunAsDefaultMovement` → `ToggleRun` (0x0A) — CORRECT, and it is NOT 0x04.**
|
|
`acclient.h:4175` is `ToggleRun_PlayerOption = 0xA`; `acclient.h:4169` is
|
|
`DisableMostWeatherEffects_PlayerOption = 0x4`. Research §2.5 (`:307`) names
|
|
the retail UI row "Run as Default Movement | 0x0A · O1 `0x00000400`" and
|
|
explicitly warns that acdream's `RuntimeMovementCommand.ToggleRunLock` is a
|
|
**keybind latch, not this option** — the implementer picked the option, not
|
|
the latch.
|
|
* **`AcceptCorpseLootingPermissions` → `AcceptLootPermits` (0x10) — CORRECT**
|
|
(`acclient.h:4181`; research §2.4 `:291` gives O1 `0x00080000`, matching
|
|
`CharacterOptionTable.cs:127`).
|
|
* **`ShowAllegianceLogons` → `DisplayAllegianceLogonNotifications` (0x18) —
|
|
CORRECT** (`acclient.h:4189`; research §2.3 `:282` gives O1 `0x08000000`,
|
|
matching `CharacterOptionTable.cs:135`).
|
|
|
|
**Count and exclusion check.** 22 + 4 = 26 entries, matching §7.3's "22 options
|
|
in tier 1 plus 4 in tier 2". No tier-3 row leaked in — spot-checked against
|
|
§5.2's exclusion list (`AttemptToDeceiveOtherPlayers` 0x09, the six privacy
|
|
rows 0x1C-0x20/0x28, `SideBySideVitals` 0x13, `AdvancedCombatUI` 0x0C,
|
|
`StayInChatMode` 0x0B, `LockUI` 0x33, `UseMouseTurning` 0x31, `ShowHelm` 0x2F,
|
|
`ShowCloak` 0x32, `HearPkDeathMessages` 0x34): none appear. The landmine ids
|
|
`0x35`/`0x36` (§5.3 constraint 2) do not exist in `CharacterOptionId` at all,
|
|
so they are unreachable by construction — correct.
|
|
|
|
**Wire-policy split:** 17 auto-save + 9 batched (`0x03, 0x06, 0x0A, 0x0D,
|
|
0x18, 0x1A, 0x22, 0x29, 0x2D`). Every one agrees with research §2's own S/B
|
|
column. No mismatch found.
|
|
|
|
---
|
|
|
|
## MUST-FIX
|
|
|
|
### MF-1 — the direct-CLI launch mode silently drops the entire declared block
|
|
|
|
`src/AcDream.Headless/Hosting/HeadlessProcessHost.cs:146-157`
|
|
|
|
```csharp
|
|
private static HeadlessSessionDescriptor WithAccount(
|
|
HeadlessSessionDescriptor source,
|
|
string account) =>
|
|
new()
|
|
{
|
|
Id = source.Id,
|
|
Endpoint = source.Endpoint,
|
|
Account = account,
|
|
Character = source.Character,
|
|
Policy = source.Policy,
|
|
Credential = source.Credential,
|
|
};
|
|
```
|
|
|
|
`HeadlessSessionDescriptor` now has **seven** properties
|
|
(`HeadlessConfiguration.cs:32-69`); this hand-rolled copy sets six. When
|
|
`--user`/`--password` are supplied (`HeadlessProcessHost.cs:78-83`,
|
|
reached from `HeadlessEntryPoint.cs:82-83` via
|
|
`HeadlessCommandLine.cs:56-95` — the K3 "direct single-session CLI launch"
|
|
mode used in the connected closeout), the descriptor handed to
|
|
`HeadlessSessionHost` has `CharacterOptions == null`.
|
|
`ParseDeclaredCharacterOptions` (`HeadlessSessionHost.cs:815-827`) then
|
|
returns an empty dictionary, `HasDeclaredOptions` is false, and
|
|
`TryDiffAndSend` early-returns forever. The config parsed, validated, and
|
|
did nothing — no exception, no diagnostic, no wire traffic.
|
|
|
|
This is directly load-bearing on the slice's own gate: OP7's connected gate is
|
|
"one live bot-vs-local-ACE run"; the test script's recipe
|
|
(`docs/research/2026-08-11-campaign-op-test-script.md`, OP7 §Recipe step 3)
|
|
uses `--config` with an environment credential, which happens to avoid the
|
|
broken path — but a coordinator who passes `--user/--password` instead would
|
|
observe a total no-op and have no signal explaining why.
|
|
|
|
**Fix:** add `CharacterOptions = source.CharacterOptions,` to `WithAccount`,
|
|
and add a regression test asserting the direct-credential path preserves it.
|
|
Consider converting `HeadlessSessionDescriptor` to a `record` so `with` makes
|
|
this class of omission impossible for the next field.
|
|
|
|
### MF-2 — a truncated-trailer PlayerDescription re-seed lets the seeder diff (and flush) against untrusted words
|
|
|
|
`src/AcDream.Runtime/Session/LiveSessionEventRouter.cs:210-215` +
|
|
`src/AcDream.Headless/Hosting/HeadlessCharacterOptionsSeeder.cs:113-136`
|
|
|
|
```csharp
|
|
onCharacterOptions: (options1, options2, trailerTruncated) =>
|
|
{
|
|
character.Character.Options.Replace(
|
|
options1, options2, armServerSeed: !trailerTruncated);
|
|
character.OnCharacterOptionsChanged?.Invoke(options1, options2); // ← fires unconditionally
|
|
},
|
|
```
|
|
|
|
OP1's R3 fix established that a `TrailerTruncated` PlayerDescription carries
|
|
**untrusted** option words ("the parser's zero placeholders, not server truth",
|
|
`RuntimeCharacterState.cs:739-745`) and therefore must not authorize a flush.
|
|
But `Replace` only ever *sets* `_hasServerSeed`, never clears it
|
|
(`RuntimeCharacterState.cs:751-756`), so after one good seed the latch stays
|
|
armed for the rest of the session. OP7 now hangs an automatic diff-and-send on
|
|
the notification that fires for truncated parses too:
|
|
|
|
1. Good PlayerDescription lands → `HasServerSeed = true`, real words.
|
|
2. A later PlayerDescription truncates. The parser reads `options1` early
|
|
(`PlayerDescriptionParser.cs:371-372`) but `options2` only after the
|
|
shortcut / 8-spell-list / desired-comps sections
|
|
(`:374-430`), so a `FormatException` anywhere in those sections
|
|
(`:450-460`) yields `options2 = 0` with `trailerTruncated = true`.
|
|
3. `Replace` installs `options2 = 0`; `HasServerSeed` stays **true**.
|
|
4. `OnCharacterOptionsChanged` fires → `NoteOptionsSeeded` →
|
|
`TryDiffAndSend` passes its `HasServerSeed` guard
|
|
(`HeadlessCharacterOptionsSeeder.cs:120-121`) and diffs against the zeroed
|
|
word: every declared Options2 id now reads "off".
|
|
5. Any declared **batched** Options2 id (`SalvageMultiple`,
|
|
`MainPackPreferred`, `ConfirmVolatileRareUse`) sets `needsFlush` →
|
|
`SaveOptions` → `TryFlush` succeeds (dirty AND seeded) →
|
|
`CharacterOptionsBlobSource.Capture` packs `character.Options.Options2`
|
|
— i.e. **zero** (`CharacterOptionsBlobSource.cs:41-42`) — into `0x01A1`.
|
|
ACE stores it raw with no validation (wire research §5.1), wiping
|
|
`AppearOffline`, all six `ListenTo*Chat`, helm/cloak and every other
|
|
Options2 bit for that character.
|
|
|
|
That is exactly the wipe class OP1's M1/R3 exist to close, re-opened from a
|
|
different direction. Without OP7 nothing dirties the module at that instant, so
|
|
this slice is what makes it reachable automatically rather than incidentally.
|
|
|
|
**Fix (cheap, at the notification seam):** carry the trust signal to the
|
|
observer — either skip the `OnCharacterOptionsChanged` invoke when
|
|
`trailerTruncated`, or widen the callback with the flag and have
|
|
`NoteOptionsSeeded` ignore untrusted seeds. A seeder-local guard alone is not
|
|
possible today: `HasServerSeed` cannot distinguish "seeded once, then
|
|
corrupted" from "seeded".
|
|
|
|
---
|
|
|
|
## SHOULD-FIX
|
|
|
|
### SF-1 — `Enum.TryParse` accepts numeric and comma-combined keys, bypassing the name allow-list
|
|
|
|
`src/AcDream.Headless/Configuration/HeadlessConfigurationLoader.cs:218-227`
|
|
|
|
```csharp
|
|
if (!Enum.TryParse(name, ignoreCase: false, out CharacterOptionId id)
|
|
|| !AllowedCharacterOptions.Contains(id))
|
|
```
|
|
|
|
`Enum.TryParse` accepts (a) a decimal numeric string and (b) a
|
|
comma-separated list of member names, OR-combining them — both behaviours apply
|
|
to non-`[Flags]` enums. The allow-list check then runs against the *resulting
|
|
value*, so a key that is not an enum-member spelling at all can pass:
|
|
|
|
* `"15"` → `0x0F` → `FellowshipShareXP` — allow-listed, accepted.
|
|
* `"ToggleRun,AutoTarget"` → `0x0A | 0x0D` = `0x0F` → **`FellowshipShareXP`** —
|
|
allow-listed, accepted. A config that reads as two movement options silently
|
|
sets fellowship XP sharing instead.
|
|
|
|
`HeadlessSessionHost.ParseDeclaredCharacterOptions` (`:815-827`) re-parses with
|
|
`Enum.Parse` and reaches the same wrong id, so nothing downstream catches it.
|
|
This is the "wrong name silently sets a different option" class D8's strictness
|
|
exists to prevent — the wire is safe (the id is real and allow-listed) but the
|
|
declaration is not honoured as written.
|
|
|
|
**Fix:** validate the *name*, not the parsed value — e.g. a
|
|
`HashSet<string>(StringComparer.Ordinal)` built from
|
|
`AllowedCharacterOptions.Select(id => id.ToString())`, or keep `TryParse` and
|
|
add `&& string.Equals(name, id.ToString(), StringComparison.Ordinal)`.
|
|
|
|
### SF-2 — a both-true fellowship declaration never converges, contradicting D8's idempotence claim
|
|
|
|
`HeadlessCharacterOptionsSeeder.cs:123-136` +
|
|
`RuntimeCharacterState.cs:818-834` (OP1's MF-2 mutual exclusion).
|
|
|
|
`IgnoreFellowshipRequests` (0x02) and `FellowshipAutoAcceptRequests` (0x12) are
|
|
both in the allow-list and each clears the other when set ON. Declaring both
|
|
`true` produces, on every single connect, in id-ascending order:
|
|
|
|
* 0x02 → set ON (recursive clear of 0x12 is a no-op if already off) → `0x0005`
|
|
* 0x12 → set ON → recursion clears 0x02 → `0x0005` (0x02=0) → `0x0005` (0x12=1)
|
|
|
|
Final state 0x02 = **off**, 0x12 = on. The next PlayerDescription echoes that,
|
|
so the declared 0x02 = true differs again and the whole dance repeats — three
|
|
wire messages per connect, forever, with the config's 0x02 declaration never
|
|
honoured. The slice's stated property ("idempotent on reconnect by
|
|
construction — the diff simply finds nothing once the server agrees", commit
|
|
message and `HeadlessCharacterOptionsSeeder.cs:47-58`) does not hold for this
|
|
config, and nothing warns the operator.
|
|
|
|
**Fix:** reject the contradiction at load (`ValidateCharacterOptions` already
|
|
has the whole dictionary in hand — both-true is a two-line check naming both
|
|
keys), which is cheaper and more honest than teaching the diff about retail's
|
|
exclusion.
|
|
|
|
### SF-3 — the "purely passive" observation hook is invoked mid-sequence in the portal path
|
|
|
|
`src/AcDream.Runtime/Session/RuntimeLiveEntitySessionController.cs:877-882`
|
|
|
|
```csharp
|
|
_session.SendGameAction(GameActionLoginComplete.Build());
|
|
_onLoginCompleteSent?.Invoke(); // ← runs the whole diff-and-send here
|
|
transit.EndTeleport();
|
|
_log(...);
|
|
```
|
|
|
|
The other site (`:199-202`) puts the invoke last in its block, which is
|
|
genuinely additive. This one sits **between** the send and
|
|
`transit.EndTeleport()`, after `_pendingPortalCompletion` was already cleared
|
|
at `:831-832`. The callback body is not trivial — it runs the full diff, real
|
|
`SendGameAction`s, `TryFlush`, and `EmitResult` → event-hub publication to
|
|
subscribed bot policies (a faulting policy observer is a modelled scenario;
|
|
see `HeadlessSessionHost.Quarantine`'s own comment at `:419-424`). A throw from
|
|
any of those aborts `EndTeleport()`, leaving the transit state machine
|
|
mid-teleport with its retry token already discarded — a new failure mode the
|
|
hook's doc comment (`:77-91`, "Never changes when or whether LoginComplete is
|
|
sent — purely additive") claims cannot exist.
|
|
|
|
**Fix:** move the invoke after `transit.EndTeleport()` (or after the `_log`),
|
|
matching the other site's tail placement.
|
|
|
|
### SF-4 — no test covers any of the three production LoginComplete → seeder hooks
|
|
|
|
The wiring tests drive the login-complete half through the
|
|
`HeadlessSessionHost.OptionsSeeder` test seam
|
|
(`HeadlessCharacterOptionsSeederWiringTests.cs:66, 109, 124, 348`) and justify
|
|
it by saying the three production sites are "each simple one-line delegate
|
|
wiring already covered by their OWN existing tests
|
|
(`RuntimeLiveEntitySessionControllerTests`)" (`:29-37`). That justification does
|
|
not hold: those tests
|
|
(`tests/AcDream.Runtime.Tests/Session/RuntimeLiveEntitySessionControllerTests.cs:149-191`)
|
|
assert the LoginComplete *bytes* are sent and construct the controller without
|
|
the new `onLoginCompleteSent` argument — the commit added no test to any of
|
|
them, and `grep onLoginCompleteSent tests/` returns nothing. So all three
|
|
call-throughs (`RuntimeLiveEntitySessionController.cs:201`, `:878`,
|
|
`HeadlessSessionHost.cs:794-800`) are unverified.
|
|
|
|
**Can the simulation hide an ordering bug?** I traced the orderings and found
|
|
none hiding today: the seeder is assigned at the *top* of `CreateEventRoute`
|
|
(`HeadlessSessionHost.cs:633-637`) before both the controller (`:722-734`) and
|
|
the bindings (`:770-783`), all three hooks read the field at invoke time (never
|
|
a captured instance), both arrival orders are unit-covered
|
|
(`HeadlessCharacterOptionsSeederTests.cs:123-167`), and generation reset clears
|
|
`HasServerSeed` per connect (`RuntimeCharacterState.cs:980-990` via
|
|
`RuntimeGenerationReset.cs:272`). The gap is regression protection, not a live
|
|
defect: a future edit that drops one of the three invokes, or wires it to the
|
|
wrong object, is invisible to the entire suite.
|
|
|
|
**Fix:** one test per site — cheapest is a `RuntimeLiveEntitySessionController`
|
|
test passing a counting `Action` and asserting it fires exactly once after each
|
|
of the two send paths, plus a host-level assertion for the first-entry
|
|
callback.
|
|
|
|
### SF-5 — the connected-gate script tells the coordinator to expect a `0x0005` that a batched id never sends
|
|
|
|
`docs/research/2026-08-11-campaign-op-test-script.md`, OP7 §"Expected wire
|
|
sends on first connect":
|
|
|
|
> One `SetSingleCharacterOption (0x0005)` for `SalvageMultiple` (id `0x22`)
|
|
> followed by exactly one `SetCharacterOptions (0x01A1)` blob flush
|
|
|
|
`SalvageMultiple` is batched (`CharacterOptionTable.cs:145`, `autoSave: false`),
|
|
and `TrySetOption` only invokes `sendAutoSave` when `entry.IsAutoSave`
|
|
(`RuntimeCharacterState.cs:836-839`) — a batched id puts **nothing** on the
|
|
wire except the later `0x01A1`. The step-5 recipe repeats the error ("exactly
|
|
ONE `0x0005` + `0x01A1` pair for that id"). The parenthetical immediately after
|
|
the first bullet gets it right ("dirties the module"), so the doc contradicts
|
|
itself. Since the gate is a WireMCP loopback capture run by the user, this
|
|
would produce a false defect report or a false pass.
|
|
|
|
**Fix:** reword both bullets — declared batched ids produce no `0x0005`; the
|
|
only observable send is the single trailing `0x01A1`.
|
|
|
|
---
|
|
|
|
## NOTE
|
|
|
|
* **N-1 — the seeder is silent and ignores every command result.**
|
|
`HeadlessCharacterOptionsSeeder.cs:131-135` discards the
|
|
`RuntimeCommandResult` from both `SetSingleOption` and `SaveOptions`, and the
|
|
class emits no diagnostic at all. A generation-mismatch or session-null
|
|
rejection (`DirectGameRuntimeCommandAdapter.cs:657-660`, `:683-686`) is
|
|
indistinguishable from "nothing to do". For a no-window bot whose gate is
|
|
"did the declared options land", one `_diagnostics.Message` line per seed
|
|
(count sent / flushed) would pay for itself. Note the partial-failure case
|
|
self-heals: a rejected `SetSingleOption` leaves local state unchanged so the
|
|
next hook re-diffs; a rejected `SaveOptions` after a successful local write
|
|
leaves the module dirty for the 480 s auto-save timer.
|
|
* **N-2 — generation captured once, before the loop**
|
|
(`HeadlessCharacterOptionsSeeder.cs:126`). If a generation change lands
|
|
mid-loop the remaining commands fail closed rather than crossing sessions.
|
|
Correct direction; worth knowing.
|
|
* **N-3 — the `OptionsSeeder` test seam is `internal`** on an `internal` type
|
|
(`HeadlessSessionHost.cs:337`), reachable only via `InternalsVisibleTo`. It
|
|
exposes `NoteLoginCompleteSent`/`NoteOptionsSeeded`, i.e. "force a diff" —
|
|
but both are idempotent and generation-gated, and the property mirrors the
|
|
pre-existing `Commands` seam. No production surface widened.
|
|
* **N-4 — doc-comment typo:** `HeadlessConfiguration.cs:60-64` says "Dictionary
|
|
**VALUES** bypass the loader's camelCase property-naming policy". It is the
|
|
dictionary **KEYS** that bypass it (the values are `bool`). The underlying
|
|
claim is correct — `JsonSerializerOptions.DictionaryKeyPolicy` is never set
|
|
(`HeadlessConfigurationLoader.cs:59-72`), so keys are read verbatim and the
|
|
PascalCase enum spelling is what the file must contain.
|
|
* **N-5 — two distinct JSON keys resolving to one id collapse silently.**
|
|
`ParseDeclaredCharacterOptions` (`HeadlessSessionHost.cs:822-825`) uses the
|
|
dictionary indexer, so `{"AutoTarget": true, "13": false}` keeps only the
|
|
last. Same root cause as SF-1; fixing SF-1 makes it unreachable.
|
|
* **N-6 — the reconnect wiring test asserts only absence**
|
|
(`HeadlessCharacterOptionsSeederWiringTests.cs:134`, `Assert.Empty`). It would
|
|
also pass if the second-session seeder were entirely dead. A flipped-value
|
|
reconnect case (declare the opposite of what session 1 persisted, assert one
|
|
send) would make it load-bearing.
|
|
* **N-7 — bookkeeping is correct as far as it goes.** "No register row" is the
|
|
right call: OP7 adds acdream-native bot tooling over wire mechanisms OP1
|
|
already ported, and introduces no retail-behaviour deviation. The §9 ledger
|
|
row for OP7 is still `—`, matching OP3's own unfilled row — per §7/§8 that is
|
|
the coordinator's post-review amend, not the implementer's omission.
|
|
|
|
---
|
|
|
|
## Lens coverage summary
|
|
|
|
| Brief item | Result |
|
|
|---|---|
|
|
| 1 — 26-name allow-list vs §5.2 tier-1+2, esp. the three risky renames | ✅ all 26 correct; table in §1. `ToggleRun` is 0x0A run-as-default, not a keybind latch and not 0x04 |
|
|
| 2 — diff engine: only differences, `SetSingleOption` then exactly one `SaveOptions`, never interleaved, reconnect-idempotent | ✅ mechanism correct (`HeadlessCharacterOptionsSeeder.cs:118-136`); the seeder **duplicates** `TrySetOption`'s unchanged-value check rather than relying on it — deliberate and beneficial (it keeps `needsFlush` from being set by a no-op). Idempotence holds except SF-2's contradictory config |
|
|
| 3 — two-precondition latch, per-connect freshness, double-fire / second-PlayerDescription races | ✅ order-independent and re-entrancy-safe (`OnCharacterOptionsChanged` fires only from the PlayerDescription route, `LiveSessionEventRouter.cs:210-215`, never from our own writes); a fresh seeder per `CreateEventRoute` plus `Options.ResetSession` on generation reset makes the latch truly per-connect; a repeat `NoteLoginCompleteSent` or a second good PlayerDescription re-diffs and finds nothing. ❌ the *truncated* second PlayerDescription is MF-2 |
|
|
| 4 — #368 compliance | ✅ zero `async`/`await`/`Task`/`ThreadPool`/`ContinueWith` added under `src/`; `Start` and every scheduler turn run on the one dedicated thread (`HeadlessProcessHost.cs:163-199`), and the affinity test measures the send from inside `ILiveSessionOperations.Tick` |
|
|
| 5 — `RuntimeLiveEntitySessionController` out-of-contract touch | Additive and null-safe (optional ctor arg, `?.Invoke()`), and **not** reachable from the graphical host — the only production construction is `HeadlessSessionHost.cs:722`; App's own LoginComplete sites (`LiveSessionRuntimeFactory.cs:268`, `LocalPlayerTeleportController.cs:301`) are untouched and pass no callback. ⚠️ but see SF-3 on the portal-path invoke position |
|
|
| 6 — HeadlessConfiguration/Loader strict-schema behaviour for existing configs | ✅ byte-identical: the new property is optional, `UnmappedMemberHandling.Disallow` / `PropertyNameCaseInsensitive = false` / camelCase policy all unchanged (`:59-72`), `CurrentVersion` still 1, and the semantic-vs-type-shape error convention is followed (`HeadlessConfigurationException` for the name, raw `JsonException` for a non-bool). ⚠️ SF-1 is a strictness hole, not a compatibility break |
|
|
| 7 — HeadlessSessionHost wiring: reconnect leaks, test-seam surface | ✅ leak-free — the only strong reference to a seeder is `_optionsSeeder`; all three hooks close over `this` and read the field at invoke time, so reassignment in `CreateEventRoute` makes the old instance collectable and no stale hook can resurrect it. Test seam: see N-3 |
|
|
| 8 — does the simulated LoginComplete half hide an ordering bug? | No live ordering bug found (trace in SF-4), but the production wiring is entirely untested — SF-4 |
|